-
Notifications
You must be signed in to change notification settings - Fork 275
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
Metadata API: Remove 3 'update' methods + tests #1736
Metadata API: Remove 3 'update' methods + tests #1736
Conversation
Pull Request Test Coverage Report for Build 1605943813
💛 - Coveralls |
Remove ambiguous, unspecific, opinionated and trivial 'update' methods, which can be replaced by feasible one-liners that assign values directly to the object attribute to be *updated*. (see theupdateframework#1627 for details). Reasons to have these methods would be increased usability in terms of - reduced work - immediate feedback on invalid assignments However, given above described issues, the reasons against the methods as they are now seem to outweigh the reasons for them. Furthermore, it seems easier to re-add similar methods, which addressed these issues, after the upcoming 1.0.0 release than to remove or modify them. This patch also removes the corresponding tests as they become irrelevant (there is no need to test object assignment). In the case of the timestamp test, the removal also includes redundant test logic, which is already tested in `test_metadata_base`. Signed-off-by: Lukas Puehringer <[email protected]>
5131962
to
f22f357
Compare
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.
Looks correct to me -- maybe we find out later that we do want some helper functions... but they will almost certainly look different so doing this ASAP seems like the right move.
the bump_*() methods have a bit of the same issue (they are not easier to use than just accessing the data structure, and bump_expiry does not do what I think repositories would want to do) but this is a move in the right direction.
Agreed. See for instance the basic repo example. There, for version bump I do the addition assignment myself...
... and for expiration "bump"s, I use a custom oneliner helper, because I needed something similar for the python-tuf/examples/repo_example/basic_repo.py Lines 52 to 54 in 1f3654f
python-tuf/examples/repo_example/basic_repo.py Lines 288 to 296 in 1f3654f
|
I'll create a ticket for those two bump methods... |
Just saw, there already is one for expiry #1727. And for bump_version I'll just drop a fix right away. |
Pull Request Test Coverage Report for Build 1605943813Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Fixes #1627
Description of the changes being introduced by the pull request:
Remove ambiguous, unspecific, opinionated and trivial 'update' methods, which can be replaced by feasible one-liners that assign values directly to the object attribute to be updated. (see #1627 for details).
Reasons to have these methods would be increased usability in terms of
However, given above described issues, the reasons against the methods as they are now seem to outweigh the reasons for them. Furthermore, it seems easier to re-add similar methods, which addressed these issues, after the upcoming 1.0.0 release than to remove or modify them.
This patch also removes the corresponding tests as they become irrelevant (there is no need to test object assignment). In the case of the timestamp test, the removal also includes redundant test logic, which is already tested in
test_metadata_base
.Please verify and check that the pull request fulfills the following
requirements: