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

Use __all__ in every public module to control which names are exported #214

Closed
federicobond opened this issue Oct 18, 2023 · 8 comments
Closed
Labels
1.0-release Required for a 1.0 release contribfest A good issue for Contribfest KubeCon EU '24 good first issue Good for newcomers
Milestone

Comments

@federicobond
Copy link
Member

No description provided.

@federicobond federicobond added good first issue Good for newcomers 1.0-release Required for a 1.0 release labels Oct 18, 2023
@gruebel
Copy link
Member

gruebel commented Jan 6, 2024

Question regarding the expectations here. Do you just want to expose all the current files/modules as is or hide in which file they actually reside.

Ex.

from openfeature import api
from openfeature.client import OpenFeatureClient
from openfeature.provider.in_memory_provider import InMemoryFlag, InMemoryProvider

or this

from openfeature import api
from openfeature import OpenFeatureClient
from openfeature.provider import InMemoryFlag, InMemoryProvider

@federicobond
Copy link
Member Author

There is room to keep improving the public module hierarchy. The only thing we should avoid is making multiple releases with breaking changes. If we are going to make changes on this then we should bundle all of them together in a single release so that users only have to migrate once.

My current thinking is that we should use __all__ to control visibility within public modules, to avoid re-exporting imports and such; and we should use underscore-prefixed modules for hiding internal structure, either using _internal modules or splitting files into multiple internal prefixed modules that get imported by a single public one.

For a good reference on this check out how the opentelemetry-python SDK does it.

As for your specific question, I don't have a strong preference either way for the OpenFeatureClient import, but the InMemoryProvider one is probably more idiomatic as openfeature.provider.in_memory. It makes sense to separate providers into multiple modules to avoid parsing/loading unused providers.

@federicobond
Copy link
Member Author

What I have said is not established convention, just my own curated best practices from years of working with Python. Feel free to suggest alternatives.

@federicobond
Copy link
Member Author

One big issue with the current module hierarchy is that the API is commonly imported as:

from openfeature import api

Or worse:

from openfeature.api import set_provider

Neither of these names (api, set_provider) give any indication as to what they belong to, if you find them in the middle of a long file. A more pythonic approach would be to be able to import them like this:

import openfeature

openfeature.set_provider(provider)

Or, if you want:

import openfeature as of

of.set_provider()

@gruebel
Copy link
Member

gruebel commented Jan 7, 2024

Typically I prefer to expose the most important ones directly at the root level of the package and less frequent used ones through the their sub namespace. But I think your suggestion can't be implemented, because openfeature is a namespace package and therefore you can't set __all__ in the __init__.py. For the sub namespaces it is no problem, but the root won't work sadly.

@federicobond
Copy link
Member Author

Yeah, I remember now that's the reason we didn't do it before.

I think a good step forward would be to compile a list of suggestions for improving the public package structure and then do these changes all in one go.

@toddbaert toddbaert added the contribfest A good issue for Contribfest KubeCon EU '24 label Mar 11, 2024
@beeme1mr beeme1mr added this to the 1.0 Release milestone Mar 27, 2024
@beeme1mr
Copy link
Member

beeme1mr commented May 1, 2024

Hey @federicobond, can this be closed now?

@gruebel
Copy link
Member

gruebel commented May 1, 2024

Yeah, this is done 😄

@gruebel gruebel closed this as completed May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0-release Required for a 1.0 release contribfest A good issue for Contribfest KubeCon EU '24 good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants