- Notifications
You must be signed in to change notification settings - Fork673
feat(api): Convert gitlab.const to Enums#1688
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedNov 11, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1688 +/- ##==========================================- Coverage 92.03% 92.02% -0.01%========================================== Files 75 76 +1 Lines 4683 4790 +107 ==========================================+ Hits 4310 4408 +98- Misses 373 382 +9
Flags with carried forward coverage won't be shown.Click here to find out more.
|
Can there be a better explanation of why this useful? Why is using: helpful? If I read that I still need to go look up what is more readable and makes the code easier to read and understand. |
Can there be a better explanation of why this useful?Why is using:```gitlab.const.AccessLevel(20)```helpful? If I read that I still need to go look up what `20` corresponds to. Seems like the current way of doing: The `gitlab.const.AccessLevel(20)` is exactly the reverse lookup youmean. Say you call``` >> Gitlab().projects.get().members.list()[0].access_level 20```And you want to map this 20 back to the level:``` >> gitlab.const.AccessLevel(20).name REPORTER_ACCESS``` ```gitlab.const.REPORTER_ACCESS```is more readable and makes the code easier to read and understand. Yes absolutely, wherever in code you should use the symbols. |
gitlabform did basically the same recently: Might be good to align so if gitlabform starts to use python-gitlab as backend it can just be reused. (sorry@gdubicki I know it's been a while 😄 ) For example, do we need |
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.
@jspricke just had a few comments regarding conventions here.
A few things are still open for me:
- need to deprecate module-level constants, looks like it could get hacky, but our next release might drop 3.6 so we may be able to use PEP 562 for this:https://stackoverflow.com/questions/45744919/subclassing-module-to-deprecate-a-module-level-variable-constant
- this should be reflected in the documentation as well, for example inhttps://github.com/python-gitlab/python-gitlab/blob/main/docs/gl_objects/access_requests.rst
- I haven't given it any thought but some basic tests might be useful 😀
Uh oh!
There was an error while loading.Please reload this page.
gitlab/const.py Outdated
| class Visibility(Enum): | ||
| VISIBILITY_PRIVATE: str = "private" | ||
| VISIBILITY_INTERNAL: str = "internal" | ||
| VISIBILITY_PUBLIC: str = "public" |
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.
Same as above, seehttps://gitlab.com/gitlab-org/gitlab/-/blob/e97357824bedf007e75f8782259fe07435b64fbb/lib/gitlab/visibility_level.rb#L23-25
| classVisibility(Enum): | |
| VISIBILITY_PRIVATE:str="private" | |
| VISIBILITY_INTERNAL:str="internal" | |
| VISIBILITY_PUBLIC:str="public" | |
| classVisibility(Enum): | |
| PRIVATE:str="private" | |
| INTERNAL:str="internal" | |
| PUBLIC:str="public" |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I'll be honest I'm not sure about this PR. Unsure what the benefit of this is. If we want to have a mapping from the integers to user-friendly names it seems like we could just add a dictionary for that. Also deprecating the current constants seems like a bad idea as it will take a LOT of work for users of this library to update their code. |
If we want to have a mapping from the integers to user-friendly names it seems like we could just add a dictionary for that. But that would duplicate the mapping from name to integers, whereasPython Enums give it for free and make sure they are consistent. |
True. I guess my biggest objection is the comment that says to deprecate the top-level values. As that is fairly user-hostile for the consumers of this library, IMHO. |
True. I guess my biggest objection is the comment that says to deprecate the top-level values. As that is fairly user-hostile for the consumers of this library, IMHO. I'm fine with leaving those in but it would mean duplicated code in themodule.I actually copied the code from the last move of the constants here:a5a48ad |
Duplicated code is okay for me in this instance as I worry that removing it would break users. |
Uh oh!
There was an error while loading.Please reload this page.
I've proposed a PR that is somewhat related to this PR: |
I wanted to say that after hearing the pros vs cons I approve of the concept of this PR. Would like some things to be cleaned up. Also the docs should be updated, though I would like if#1694 lands first. |
Thanks for all the proposals and comments. I hope I added all of them in the new version. |
@jspricke Had a question. Currently we do something like this: How will that look using the enums in replacing |
Most likely |
There have been some updates to prepare for this PR as we were importing all constants into the top-level namespace,@jspricke would you mind rebasing this? |
Rebased. |
JohnVillalovos left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. Looks good to me.
Not merging as I think@nejch had some changes wanted.
Thanks again@jspricke. As I mentioned earlier we reference the old constants in a lot of our documentation. Was your idea to fully replace them eventually? If so we should also update the docs: |
Thanks again@jspricke. As I mentioned earlier we reference the old constants in a lot of our documentation. Was your idea to fully replace them eventually? If so we should also update the docs: Yes, updated. Sorry for taking so long. |
I've just rebased this to fix a merge conflict. Anything I could do to get this merged? |
Erotemic commentedJun 21, 2022
Chiming in with my $0.02. I just started using the gitlab API, and was trying to figure out how to specify access controls. Having enums like this instead of the existing constants makes a lot of sense to me. I was hoping that there would be an open issue / PR about it and I'm glad there is. I do hope this gets merged soon. That being said, from an existing user perspective, it is a pain to transition away from anything existing even if there is a better option, especially if there is any intention to eventually remove the existing constants. Fortunately, we have Python 3.7's PEP 562 to the rescue: module level I suggest that you add something like the following code which will warn users when they use the old constants instead of the new constants: def__getattr__(key):importwarnings_DEPRECATED_CONST=dict(NO_ACCESS=AccessLevel.NO_ACCESS.value,MINIMAL_ACCESS=AccessLevel.MINIMAL_ACCESS.value,GUEST_ACCESS=AccessLevel.GUEST.value,REPORTER_ACCESS=AccessLevel.REPORTER.value,DEVELOPER_ACCESS=AccessLevel.DEVELOPER.value,MAINTAINER_ACCESS=AccessLevel.MAINTAINER.value,OWNER_ACCESS=AccessLevel.OWNER.value,#VISIBILITY_PRIVATE=Visibility.PRIVATE.value,VISIBILITY_INTERNAL=Visibility.INTERNAL.value,VISIBILITY_PUBLIC=Visibility.PUBLIC.value,#NOTIFICATION_LEVEL_DISABLED=NotificationLevel.DISABLED.value,NOTIFICATION_LEVEL_PARTICIPATING=NotificationLevel.PARTICIPATING.value,NOTIFICATION_LEVEL_WATCH=NotificationLevel.WATCH.value,NOTIFICATION_LEVEL_GLOBAL=NotificationLevel.GLOBAL.value,NOTIFICATION_LEVEL_MENTION=NotificationLevel.MENTION.value,NOTIFICATION_LEVEL_CUSTOM=NotificationLevel.CUSTOM.value,# Search scopes,# all scopes (global, group and project),SEARCH_SCOPE_PROJECTS=SearchScope.PROJECTS.value,SEARCH_SCOPE_ISSUES=SearchScope.ISSUES.value,SEARCH_SCOPE_MERGE_REQUESTS=SearchScope.MERGE_REQUESTS.value,SEARCH_SCOPE_MILESTONES=SearchScope.MILESTONES.value,SEARCH_SCOPE_WIKI_BLOBS=SearchScope.WIKI_BLOBS.value,SEARCH_SCOPE_COMMITS=SearchScope.COMMITS.value,SEARCH_SCOPE_BLOBS=SearchScope.BLOBS.value,SEARCH_SCOPE_USERS=SearchScope.USERS.value,# specific global scope,SEARCH_SCOPE_GLOBAL_SNIPPET_TITLES=SearchScope.GLOBAL_SNIPPET_TITLES.value,# specific project scope,SEARCH_SCOPE_PROJECT_NOTES=SearchScope.PROJECT_NOTES.value, )_DEPRECATED_REPL=dict(NO_ACCESS='AccessLevel.NO_ACCESS.value',MINIMAL_ACCESS='AccessLevel.MINIMAL_ACCESS.value',GUEST_ACCESS='AccessLevel.GUEST.value',REPORTER_ACCESS='AccessLevel.REPORTER.value',DEVELOPER_ACCESS='AccessLevel.DEVELOPER.value',MAINTAINER_ACCESS='AccessLevel.MAINTAINER.value',OWNER_ACCESS='AccessLevel.OWNER.value',#VISIBILITY_PRIVATE='Visibility.PRIVATE.value',VISIBILITY_INTERNAL='Visibility.INTERNAL.value',VISIBILITY_PUBLIC='Visibility.PUBLIC.value',#NOTIFICATION_LEVEL_DISABLED='NotificationLevel.DISABLED.value',NOTIFICATION_LEVEL_PARTICIPATING='NotificationLevel.PARTICIPATING.value',NOTIFICATION_LEVEL_WATCH='NotificationLevel.WATCH.value',NOTIFICATION_LEVEL_GLOBAL='NotificationLevel.GLOBAL.value',NOTIFICATION_LEVEL_MENTION='NotificationLevel.MENTION.value',NOTIFICATION_LEVEL_CUSTOM='NotificationLevel.CUSTOM.value',# Search scopes',# all scopes (global', group and project)',SEARCH_SCOPE_PROJECTS='SearchScope.PROJECTS.value',SEARCH_SCOPE_ISSUES='SearchScope.ISSUES.value',SEARCH_SCOPE_MERGE_REQUESTS='SearchScope.MERGE_REQUESTS.value',SEARCH_SCOPE_MILESTONES='SearchScope.MILESTONES.value',SEARCH_SCOPE_WIKI_BLOBS='SearchScope.WIKI_BLOBS.value',SEARCH_SCOPE_COMMITS='SearchScope.COMMITS.value',SEARCH_SCOPE_BLOBS='SearchScope.BLOBS.value',SEARCH_SCOPE_USERS='SearchScope.USERS.value',# specific global scope',SEARCH_SCOPE_GLOBAL_SNIPPET_TITLES='SearchScope.GLOBAL_SNIPPET_TITLES.value',# specific project scope',SEARCH_SCOPE_PROJECT_NOTES='SearchScope.PROJECT_NOTES.value', )ifkeyin_DEPRECATED_CONST:warnings.warn('The constant gitlab.const.{key} was deprecated in version TBD. ''and will be removed in version TBD. ''Use gitlab.const.{_DEPRECATED_REPL[key]} instead. ')return_DEPRECATED[key]raiseAttributeError(key) |
@Erotemic this was essentially my thinking in#1688 (review) as well :) sorry for the delays here, I'll take a look now and rebase this as@jspricke has already been waiting a while. |
This allows accessing the elements by value, i.e.:import gitlab.constgitlab.const.AccessLevel(20)
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.
This allows accessing the elements by value, i.e.:
import gitlab.const
gitlab.const.AccessLevel(20)