-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add default args to Signed constructors #1922
Conversation
This allows creating new metadata with less boilerplate: root = Metadata(Root()) targets = Metadata(Targets()) Set reasonable default values for all the arguments -- version to 1, spec_version to current supported version, etc. Expires does not have a good default value and my original plan was to require expires argument to be set. That would mean an incompatible API change though as arguments before expires would be now optional... So expires now defaults to an arbitrary value of 1 day from moment of creation. One noteworthy special case is consistent_snapshot where the default value is True (since that's what we want people to use for new metadata) but None is also used to imply that metadata does not contain the field at all. Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
Signed-off-by: Jussi Kukkonen <[email protected]>
The generated tests and serialization tests do a good job testing this I believe.
Metadata docstring should maybe be improved to show an example of creating a new Metadata... |
Pull Request Test Coverage Report for Build 2089353376
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the changes are great with a few comments.
I am wondering whether we should simplify and use the default attribute values in the examples
scripts?
For those examples, I think we all agree the most important priority is that the code is easy to understand.
Otherwise, the simplifications are great, no doubt.
md_timestamp = Metadata(Timestamp(expires=EXPIRY)) | ||
md_snapshot = Metadata(Snapshot(expires=EXPIRY)) | ||
md_targets = Metadata(Targets(expires=EXPIRY)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like you don't have to create temporary Signed
objects but you can't directly instantiate a Metadata object with populated default Signed
fields as well.
Really cool!
Signed-off-by: Jussi Kukkonen <[email protected]>
If argument is an empty container, we want to use the given empty container. Only create a new container if argument is None. Signed-off-by: Jussi Kukkonen <[email protected]>
This means the metadata is by default expired: this seems like a fine default since we only allow a default value for practical reasons (not allowing it would mean backwards incompatible API change). Signed-off-by: Jussi Kukkonen <[email protected]>
I believe all comments we're resolved with additional commits:
|
I believe this is done, if you have specific place you're thinking of let me know. |
I am sorry, I think I was unclear. |
If there is a specific case where this is true I'm willing to discuss. I just don't see one, everything seems to have a fairly obvious default value apart from expiry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the change! Please fix typo (see inline) and this is good to go.
The comment about the docstring is not a blocker nor is this one:
Re beeing more careful with empty containers (d8c0f3b), should we also update all the unrecognized_fields or {}
in Metadata constructors?
8148efb
to
8039d99
Compare
Force pushed with the documentation commit amended with three fixes for issues pointed out in comments (the last three resolved discussions).
I'll file an issue: it's pretty minor but strictly speaking yes we should |
Signed-off-by: Jussi Kukkonen <[email protected]>
8039d99
to
0d3bb68
Compare
Once more, force-pushed with a fix for latest resolved review comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Tweak the constructor signatures to make creating brand new Metadata easier (the constructors were originally designed for deserialization purposes). Modify examples and tests to use the new constructor args.
The changes are backwards compatible. Resulting API is much easier to use for creating new Metadata. The API is still a bit unusual but not difficult to learn IMO:
There are some noteworthy cases:
expires
does not have a reasonable default value and I planned to leave it a required argument. That would have broken backwards compat however as order of arguments would have had to change. So expires now defaults to an arbitrary 1 day in future EDIT: changed toutcnow()
so default is expired metadataconsistent_snapshot
defaults to True but None is still possible to indicate Metadata does not contain the field at allspec_version
defaults to whatever is the current library supported versionOnly the core metadata classes we're updated. There are other classes that would probably benefit from a better constructor, like Delegations, but those were not handled yet.
Fixes #1459
Please verify and check that the pull request fulfills the following
requirements: