-
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: Consider removing 'update()' methods #1627
Comments
The case for (better named) update methods is that they do allow us to do validation on user input, while inserting things directly to a dictionary does not. This is a major advantage in an untyped language but, I think, made slightly less important with good static typing support: calling code is still free to insert nonsense into our Metadata data structures, but static typing checks will inform them that they are doing bad things (at least it will once we fix #1633). We may still want to do the input validation but I would argue it's not as critical as it would be otherwise. |
Added one concern from #1230 to the issue description above. Will close there. |
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]>
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]>
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]>
[edited on 12/20/2021: Mention ".json" bias]
[edited on 11/10/2021: Point to #1230]
Description of issue or feature request:
The Metadata API classes for top-level roles Targets, Timestamp and Snapshot each have an
update
method. They have been criticised in several places (#1193 (comment) pp, #1223 (comment) pp, #1620 (comment)).More specifically the following doubts have been expressed:
update
is an ambiguous name, e.g. does it stand for "update some attributes of object" or for "update object in the course of a repository update"? - New metadata API: add MetadataInfo and TargetFile classes #1223 (comment)update
is unspecific about which attributes are updated - New API: Revise "update" methods in tuf/api/metadata.py #1230Is
update
a repository action only and should thus be in a repository controller and not on the Metadata API, which is also used by the client? - How much metadata behaviour should be implemented on their classes? #1134[e.g.
Targets.update()
] is literally a one-liner that modifies a public dict attribute: we could document doing that instead and have a smaller API surface. - Add missing method args docs in metadata API #1620 (comment)[e.g.
Targets.update()
] inserts or replaces the TargetFile... but a Targets handles both TargetFiles and delegations: what if I wanted to modify delegations instead? is it obvious that TargetFile needs a function but delegations do not? - Add missing method args docs in metadata API #1620 (comment)[e.g.
Snapshot.update()
] is opinionated in regards to the file extension of the targets metadata to be updated with, i.e. it assumes that it is.json
. The rest of the metadata API also often defaults to json, but allows a caller to override, which is not the case here.Current behavior:
Expected behavior:
The text was updated successfully, but these errors were encountered: