feat(api): Convert gitlab.const to Enums by jspricke · Pull Request #1688 · python-gitlab/python-gitlab · GitHub

This allows accessing the elements by value, i.e.:

import gitlab.const
gitlab.const.AccessLevel(20)

Codecov Report

Merging #1688 (587642e) into main (2708f91) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            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

Flag	Coverage Δ
cli_func_v4	81.46% <100.00%> (+0.03%)	⬆️
py_func_v4	80.83% <100.00%> (-0.21%)	⬇️
unit	83.34% <100.00%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
gitlab/const.py	100.00% <100.00%> (ø)
gitlab/v4/objects/notification_settings.py	96.42% <0.00%> (-3.58%)	⬇️
gitlab/v4/objects/merge_request_approvals.py	90.58% <0.00%> (-2.01%)	⬇️
gitlab/cli.py	98.16% <0.00%> (-1.84%)	⬇️
gitlab/v4/cli.py	81.38% <0.00%> (-0.95%)	⬇️
gitlab/v4/objects/__init__.py	100.00% <0.00%> (ø)
gitlab/v4/objects/projects.py	87.19% <0.00%> (ø)
gitlab/v4/objects/statistics.py	100.00% <0.00%> (ø)
gitlab/v4/objects/topics.py	100.00% <0.00%> (ø)
gitlab/client.py	90.10% <0.00%> (+0.05%)	⬆️
... and 3 more

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:

gitlab.const.REPORTER_ACCESS

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:

https://github.com/egnyte/gitlabform/blob/e58fac8f288fb10584c2eff0e5da2aec1c77b695/gitlabform/gitlab/__init__.py#L20-L33

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 OWNER_ACCESS instead of just OWNER if it's already scoped inside AccessLevel?
If I get this right though, people will need to use gitlab.const.AccessLevel.REPORTER.value to access it.

@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 😀

Same as above, see https://gitlab.com/gitlab-org/gitlab/-/blob/e97357824bedf007e75f8782259fe07435b64fbb/lib/gitlab/visibility_level.rb#L23-25

Suggested change

      
            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.

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

I'm fine with leaving those in but it would mean duplicated code in the module.

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:
#1694

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:

member = project.members.create({'user_id': user.id, 'access_level':
                                 gitlab.const.DEVELOPER_ACCESS})

How will that look using the enums in replacing gitlab.const.DEVELOPER_ACCESS?

@jspricke Had a question. Currently we do something like this:
member = project.members.create({'user_id': user.id, 'access_level':
                                 gitlab.const.DEVELOPER_ACCESS})
How will that look using the enums in replacing gitlab.const.DEVELOPER_ACCESS?

Most likely gitlab.const.AccessLevel.DEVELOPER.value I assume, though I think there's a way to make it directly accessible via gitlab.const.AccessLevel.DEVELOPER from what I see here https://github.com/encode/httpx/blob/master/httpx/_status_codes.py

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.

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:

$ grep -r 'const\.' docs/
docs/gl_objects/access_requests.rst:* ``gitlab.const.GUEST_ACCESS``: ``10``
docs/gl_objects/access_requests.rst:* ``gitlab.const.REPORTER_ACCESS``: ``20``
docs/gl_objects/access_requests.rst:* ``gitlab.const.DEVELOPER_ACCESS``: ``30``
docs/gl_objects/access_requests.rst:* ``gitlab.const.MAINTAINER_ACCESS``: ``40``
docs/gl_objects/access_requests.rst:* ``gitlab.const.OWNER_ACCESS``: ``50``
docs/gl_objects/access_requests.rst:    ar.approve(access_level=gitlab.const.MAINTAINER_ACCESS)  # explicitly set access level
docs/gl_objects/groups.rst:    group.share(group2.id, gitlab.const.DEVELOPER_ACCESS)
docs/gl_objects/groups.rst:* ``gitlab.const.GUEST_ACCESS = 10``
docs/gl_objects/groups.rst:* ``gitlab.const.REPORTER_ACCESS = 20``
docs/gl_objects/groups.rst:* ``gitlab.const.DEVELOPER_ACCESS = 30``
docs/gl_objects/groups.rst:* ``gitlab.const.MAINTAINER_ACCESS = 40``
docs/gl_objects/groups.rst:* ``gitlab.const.OWNER_ACCESS = 50``
docs/gl_objects/groups.rst:                                   'access_level': gitlab.const.GUEST_ACCESS})
docs/gl_objects/groups.rst:    member.access_level = gitlab.const.DEVELOPER_ACCESS
docs/gl_objects/groups.rst:    group.add_ldap_group_link(ldap_group_cn, gitlab.const.DEVELOPER_ACCESS, 'ldapmain')
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_DISABLED``
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_PARTICIPATING``
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_WATCH``
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_GLOBAL``
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_MENTION``
docs/gl_objects/notifications.rst:* ``gitlab.const.NOTIFICATION_LEVEL_CUSTOM``
docs/gl_objects/notifications.rst:    settings.level = gitlab.const.NOTIFICATION_LEVEL_WATCH
docs/gl_objects/notifications.rst:    settings.level = gitlab.const.NOTIFICATION_LEVEL_CUSTOM
docs/gl_objects/projects.rst:* ``gitlab.const.VISIBILITY_PRIVATE``
docs/gl_objects/projects.rst:* ``gitlab.const.VISIBILITY_INTERNAL``
docs/gl_objects/projects.rst:* ``gitlab.const.VISIBILITY_PUBLIC``
docs/gl_objects/projects.rst:                                       gitlab.const.VISIBILITY_PRIVATE})
docs/gl_objects/projects.rst:                                     gitlab.const.DEVELOPER_ACCESS})
docs/gl_objects/projects.rst:    member.access_level = gitlab.const.MAINTAINER_ACCESS
docs/gl_objects/projects.rst:    project.share(group.id, gitlab.const.DEVELOPER_ACCESS)
docs/gl_objects/protected_branches.rst:        'merge_access_level': gitlab.const.DEVELOPER_ACCESS,
docs/gl_objects/protected_branches.rst:        'push_access_level': gitlab.const.MAINTAINER_ACCESS
docs/gl_objects/protected_branches.rst:        'allowed_to_unprotect': [{"access_level": gitlab.const.MAINTAINER_ACCESS}]
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_PROJECTS``: ``projects``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_ISSUES``: ``issues``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_MERGE_REQUESTS``: ``merge_requests``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_MILESTONES``: ``milestones``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_WIKI_BLOBS``: ``wiki_blobs``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_COMMITS``: ``commits``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_BLOBS``: ``blobs``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_USERS``: ``users``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_GLOBAL_SNIPPET_TITLES``: ``snippet_titles``
docs/gl_objects/search.rst:  + ``gitlab.const.SEARCH_SCOPE_PROJECT_NOTES``: ``notes``
docs/gl_objects/search.rst:    gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, 'regression')
docs/gl_objects/search.rst:    group.search(gitlab.const.SEARCH_SCOPE_ISSUES, 'regression')
docs/gl_objects/search.rst:    project.search(gitlab.const.SEARCH_SCOPE_ISSUES, 'regression')
docs/gl_objects/search.rst:    gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, page=2, per_page=10)
docs/gl_objects/search.rst:    for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, as_list=False):
docs/gl_objects/search.rst:    for item in gl.search(gitlab.const.SEARCH_SCOPE_ISSUES, search_str, as_list=False):
docs/gl_objects/snippets.rst:    snippet.visibility_level = gitlab.const.VISIBILITY_PUBLIC

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 __getattr__.

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)

Thanks again @jspricke for your patience and rework here and also @Erotemic for your ping. This needs a little more on the testing/deprecation side but we'll do it in a follow-up.

jspricke force-pushed the enum branch from 9b0331f to 662744a Compare November 11, 2021 22:35

jspricke changed the title Convert gitlab.const to Enums feat(api): Convert gitlab.const to Enums Nov 11, 2021

jspricke force-pushed the enum branch from 662744a to 27e839a Compare November 12, 2021 07:27

nejch requested changes Nov 16, 2021

View reviewed changes

nejch reviewed Nov 16, 2021

View reviewed changes

Comment thread gitlab/const.py Outdated

JohnVillalovos reviewed Nov 16, 2021

View reviewed changes

Comment thread gitlab/const.py Outdated

jspricke force-pushed the enum branch from 27e839a to cf84b4b Compare November 17, 2021 22:45

jspricke force-pushed the enum branch from cf84b4b to ba2539e Compare November 17, 2021 22:52

jspricke force-pushed the enum branch from ba2539e to 534a00f Compare December 2, 2021 07:29

JohnVillalovos approved these changes Dec 2, 2021

View reviewed changes

jspricke force-pushed the enum branch from 534a00f to 587642e Compare December 21, 2021 10:14

jspricke force-pushed the enum branch from 587642e to ce106c7 Compare April 5, 2022 07:51

feat(api): convert gitlab.const to Enums

c3c6086

This allows accessing the elements by value, i.e.: import gitlab.const gitlab.const.AccessLevel(20)

nejch force-pushed the enum branch from ce106c7 to c3c6086 Compare June 22, 2022 20:06

nejch approved these changes Jun 22, 2022

View reviewed changes

nejch merged commit f0ac3cd into python-gitlab:main Jun 22, 2022

jspricke deleted the enum branch June 23, 2022 06:21

feat(api): Convert gitlab.const to Enums by jspricke · Pull Request #1688 · python-gitlab/python-gitlab · GitHub

Repository navigation

feat(api): Convert gitlab.const to Enums - #1688

feat(api): Convert gitlab.const to Enums#1688nejch merged 1 commit intopython-gitlab:mainpython-gitlab/python-gitlab:mainfrom jspricke:enumjspricke/python-gitlab:enumCopy head branch name to clipboard

Conversation

jspricke commented Nov 11, 2021

Uh oh!

codecov-commenter commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JohnVillalovos commented Nov 13, 2021

Uh oh!

jspricke commented Nov 13, 2021 via email

Uh oh!

nejch commented Nov 15, 2021

Uh oh!

nejch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nejch Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JohnVillalovos commented Nov 16, 2021

Uh oh!

jspricke commented Nov 16, 2021 via email

Uh oh!

JohnVillalovos commented Nov 16, 2021

Uh oh!

jspricke commented Nov 16, 2021 via email

Uh oh!

JohnVillalovos commented Nov 16, 2021

Uh oh!

Uh oh!

JohnVillalovos commented Nov 17, 2021

Uh oh!

JohnVillalovos commented Nov 17, 2021

Uh oh!

jspricke commented Nov 17, 2021

Uh oh!

JohnVillalovos commented Nov 17, 2021

Uh oh!

nejch commented Nov 18, 2021

Uh oh!

nejch commented Dec 1, 2021

Uh oh!

jspricke commented Dec 2, 2021

Uh oh!

JohnVillalovos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nejch commented Dec 11, 2021

Uh oh!

jspricke commented Dec 21, 2021 via email

Uh oh!

jspricke commented Apr 5, 2022

Uh oh!

Erotemic commented Jun 21, 2022

Uh oh!

nejch commented Jun 22, 2022

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat(api): Convert gitlab.const to Enums#1688
nejch merged 1 commit into
python-gitlab:mainfrom
jspricke:enum

codecov-commenter commented Nov 11, 2021 •
edited
Loading

nejch left a comment •
edited
Loading

JohnVillalovos left a comment •
edited
Loading