Release note suggested changes #2647
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is unnecessary, as we can simply set the field to `null` to simplify the response type interface
This has the benefits of not having to pass `release_notes` around through multiple nested function calls + trusting celery's python string (de)serialization functionality (since we were placing the entire `release_notes` string onto the queue before).
- Remove `test_version_publish_without_release_notes` test. This is just testing the existing publish functionality, which we already have tests for. - Fix `test_version_build_publishable_with_release_notes` so that it passes - Remove `test_version_rest_publish_empty_release_notes` test, as it is no longer applicable. `Version.release_notes` is no longer a nullable field, instead defaulting to empty string (best practice for CharField)
Member
|
FWIW we have already released 0.12.0 (and 0.12.1) of dandi-schema after which should support release notes in metadata and master has that since so the "foundation" should be ready |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR functions as a "requested changes" review for #2606.
Changes from the design in the original PR:
In the version response serializer, instead of making the presence of the
release_notesfield conditional on whether a Version has release notes or not, it always includes it, simply returning an empty string if there are no release notes. This is a cleaner API design and makes things such as Swagger/OpenAPI specs easier to generate.Moved release notes to be stored on the
Versionmodel directly, and then write it to the metadata upon publish (very similar to howdoiworks currently, but obviously slightly different semantics). This simplifies the code significantly, as we're no longer having to pass the release_notes string through several layers of function calls. Additionally, this avoids having to place the entirerelease_notesstring on the RabbitMQ queue; from what I understand, doing that is technically fine, but usually we try to avoid that pattern and only place database primary keys on the queue.