Skip to content
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

Export all public version classes at package level #105

Merged
merged 1 commit into from
May 2, 2023

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Apr 21, 2023

By exporting all the public version classes that consumers are allowed to use globally in __init__.py it will make it much easier to use them given that you don't have to import specific modules:

from mozilla_version import MobileVersion

A real use-case example you can find at: mozilla/mozdownload#632 (comment)

@jcristau this is what we discussed on Matrix two days ago. I hope that it's clearer now.

@whimboo whimboo changed the title Export all public version classes on package level Export all public version classes at package level Apr 21, 2023
@whimboo
Copy link
Contributor Author

whimboo commented Apr 24, 2023

CC @JohanLorenzo as well.

Copy link
Member

@gabrielBusta gabrielBusta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this LGTM

@whimboo
Copy link
Contributor Author

whimboo commented May 2, 2023

@jcristau or @JohanLorenzo could anyone from you please review? Thanks.

@jcristau
Copy link
Collaborator

jcristau commented May 2, 2023

I can't say I'm a fan, to be honest.

Copy link
Contributor

@JohanLorenzo JohanLorenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting up this patch @whimboo! I personally don't have any strong preferences on whether we want to expose these classes to the top-level of mozilla-version. That said, I understand this may be hard for newcomers to find which class to use. Thus, I'm okay to make this change.

I chatted with @jcristau. A good compromise would be to expose only the supported products. I left details on which classes to take out. Let me know what you think!

mozilla_version/__init__.py Outdated Show resolved Hide resolved
mozilla_version/__init__.py Outdated Show resolved Hide resolved
mozilla_version/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JohanLorenzo JohanLorenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@JohanLorenzo JohanLorenzo merged commit 892cf8c into mozilla-releng:main May 2, 2023
@whimboo whimboo deleted the unify_version_classes branch May 2, 2023 11:45
@whimboo
Copy link
Contributor Author

whimboo commented May 23, 2023

Any chance a new minor release could be done so that those changes can be used by other projects? Thanks.

@JohanLorenzo
Copy link
Contributor

JohanLorenzo commented May 23, 2023

@whimboo
Copy link
Contributor Author

whimboo commented May 24, 2023

Thanks a lot @JohanLorenzo! We appreciate. Once the contributor updated the patch I will let you know. But locally it was already working for me when I've created this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants