-
Notifications
You must be signed in to change notification settings - Fork 643
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
fix: Added const module to base namespace. #2820
Conversation
See #2819. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2820 +/- ##
==========================================
- Coverage 96.52% 96.51% -0.02%
==========================================
Files 90 90
Lines 5872 5874 +2
==========================================
+ Hits 5668 5669 +1
- Misses 204 205 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Thanks for the PR @valentingregoire! AFAIK this was an intentional change at some point to enforce namespacing the constants, as the number of package-level exports kept growing. Or am I forgetting something, do you remember @JohnVillalovos? In any case I don't mind this but perhaps we should avoid exporting the deprecated strings that have been replaced by enums. |
That is what I remember @nejch |
@nejch, @JohnVillalovos, I see that this could be the intention, indeed. Would it then make sense to add the |
@valentingregoire modules are not exported in You simply do one of the folllowing: # module import with full namespace
import gitlab.const
gitlab.const.AccessLevel.ADMIN
<AccessLevel.ADMIN: 60>
# module import
from gitlab import const
const.AccessLevel.ADMIN
<AccessLevel.ADMIN: 60>
# class import
from gitlab.const import AccessLevel
AccessLevel.ADMIN
<AccessLevel.ADMIN: 60> Does this not work for you? |
@nejch, yes that works perfectly fine. It's just that it's a bit inconsistent with the exceptions that are added to the global namespace, where the constants are not. For example: from gitlab import GitlabGetError
from gitlab.const import MAINTAINER_ACCESS This feels a bit weird and inconsistent. Maybe the correct solution is either to leave everything as is, or to put the exceptions out of the global namespace. This would be another major change though... |
@valentingregoire a lot of python libraries seem to export exceptions at the root package level as a convention, but true if we want to be really consistent here we would leave them out in just |
@valentingregoire I'm leaning towards leaving this as is, please consider #2820 (comment) if you disagree and would like to go ahead and reopen this PR 🙇 Thanks again for your time! |
I agree, we can leave it as is 👍.
Op wo 29 mei 2024 22:20 schreef Nejc Habjan ***@***.***>:
… @valentingregoire <https://github.com/valentingregoire> I'm leaning
towards leaving this as is, please consider #2820 (comment)
<#2820 (comment)>
if you disagree and would like to go ahead and reopen this PR 🙇
Thanks again for your time!
—
Reply to this email directly, view it on GitHub
<#2820 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHYZ7UFOHA23HQZRBDLMSB3ZEY2BZAVCNFSM6AAAAABESLEUDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYGE4TOOJRGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Changes
Documentation and testing
Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated: