-
Notifications
You must be signed in to change notification settings - Fork 278
Create constants for top-level rolenames #1672
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
Conversation
010dcc8
to
13d0345
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.
Pull Request Test Coverage Report for Build 1452565760Warning: 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 |
I agree with both of you ...
On the other hand, names do grow in size. I was thinking if it there is a convenient data structure than would:
But this is only a suggestion and if implementing this feels like an overkill for python-tuf, I'm ok with simple constants. |
@lukpueh do you have an opinion on the matter? |
FWIW, in warehouse they use an enum subclass to do this, maybe that's the data structure @sechkova was thinking of, it seems to at least allow iterating etc.? :) Another way of encapsulation would be to make them class attributes of the corresponding metadata classes. I'm really fine either way. |
I think enum covers all the cases, as @lukpueh also mentioned. |
13d0345
to
3a7e025
Compare
I pushed an update with enum. |
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.
Honestly, I am still uncertain about the usage of an enum here compared to using separate constants, and given that they will be uppercase I don't think they will be mistaken with something else.
Still, if we decide to use enums I have two suggestions:
- when defining RoleNames make sure you inherit
str
, this will shorten the comparisons as I explained in my comment - when iterating all of the roles make sure to do:
for role in RoleNames
instead of mentioning each of the roles explicitly.
I left a comment about that too.
EDIT: I think you can directly just inherit StrEnum
with the same success as inheriting Enum
and str
separatly.
For more info: https://docs.python.org/3/library/enum.html#others
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 don't have opinions on the actual data structure but I have a couple on the usage:
- a few legacy test files are modified: The split between legacy and metadata API is confusing so not your fault, but let's not start mixing the two implementations. Basically if the file did not import
tuf.api.metadata
already, it's probably legacy and we don't want to modify the file. so test_repository_tool, test_sig, test_updater should not be touched - It's great that we try to avoid typos by avoiding hard-coded strings... but it should not come at at the expense of readability: I think going from
"root"
toRoleNames.ROOT.value
is not a clear cut win as the latter is more than 3 times as long (!) and looks so complex -- I could not guess from the name that I'm looking at a simple role name.
It seems like the major reason to use an enum or a class is to avoid top level variable name pollution (so to avoid What if instead of a new class we used the classes we are already importing? Root and others already have a class variable (for reference: the class variable _signed_type is probably currently private to make it very clear it should not be modified, but as python does not have "class properties" that means we can't keep using a property in this case(?). I think solving the problem described in this issue is more useful than keeping the type private) |
This is what I meant above with ...
One downside would be that we'd have to define a global |
I'm also not going to lose sleep over a single instance of duplicating the strings within the API implementation if that makes the API definition look neater: what's important is removing the magic strings from the code that imports |
3a7e025
to
a6c8c73
Compare
I've put the constants within the role classes. Agree that it's more clear, simple and convenient. As those are both required private (we don't want to be able to change them, whatever "be able" means in python for private properties :D ), but at the same time we need them for static reference within Updater, and we don't want to break the file format which uses |
699972c
to
1c0c253
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.
LGTM, left a couple of comments but I think none were blockers: please take a look and adapt or don't (leave a comment if you don't), either way works
I think the genius idea is to just remove |
Agreed!
I actually suspect that in this case the (Python) implementation influenced the naming decision in the spec, to avoid shadowing the Regardless of this specific case, I still think that it very important that the spec file format is recognisable in the reference implementation. |
This is a change in the metadata API to remove hardcoded rolenames and use constants instead. Fixes theupdateframework#1648 Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
This applies the use of constants of top-level rolenames in the tests instead of the previously hardcoded strings. Fixes theupdateframework#1648 Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
1c0c253
to
00589f0
Compare
Thanks @jku. Just addressed your comments. |
Pull Request Test Coverage Report for Build 1471534805Warning: 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 #1648
This is a change in the metadata API to remove hardcoded role names
and use constants instead.