feat(api): Convert gitlab.const to Enums #1688
Conversation
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 you
mean. 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.
@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 in https://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 😀
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.
Same as above, see https://gitlab.com/gitlab-org/gitlab/-/blob/e97357824bedf007e75f8782259fe07435b64fbb/lib/gitlab/visibility_level.rb#L23-25
| class Visibility(Enum): | |
| VISIBILITY_PRIVATE: str = "private" | |
| VISIBILITY_INTERNAL: str = "internal" | |
| VISIBILITY_PUBLIC: str = "public" | |
| class Visibility(Enum): | |
| PRIVATE: str = "private" | |
| INTERNAL: str = "internal" | |
| PUBLIC: str = "public" |
|
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, whereas
Python 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 the
module.
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. |
|
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. |
There was a problem hiding this comment.
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? |
|
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):
import warnings
_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',
)
if key in _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]
raise AttributeError(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.const gitlab.const.AccessLevel(20)
This allows accessing the elements by value, i.e.:
import gitlab.const
gitlab.const.AccessLevel(20)