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

glass namespace contains unwanted symbols #491

Open
ntessore opened this issue Jan 28, 2025 · 4 comments
Open

glass namespace contains unwanted symbols #491

ntessore opened this issue Jan 28, 2025 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@ntessore
Copy link
Collaborator

Describe the Bug

These add noise to the glass namespace:

import contextlib
from importlib.metadata import PackageNotFoundError

with contextlib.suppress(PackageNotFoundError):
    from ._version import __version__, __version_tuple__

and try: ... catch ImportError: pass should work just as well?

To Reproduce

import glass
print(glass.contextlib, glass.PackageNotFoundError)

Expected Behaviour

Not have contextlib and PackageNotFoundError in the glass namespace.

Actual Behaviour

Have contextlib and PackageNotFoundError in the glass namespace.

Version In Use

2024.3

Additional Context

@ntessore ntessore added the bug Something isn't working label Jan 28, 2025
@Saransh-cpp
Copy link
Member

We should actually not be suppressing that error, instead we should let import glass fail if . _version.__version__ is not present.

@Saransh-cpp Saransh-cpp self-assigned this Jan 30, 2025
@ntessore
Copy link
Collaborator Author

I think we need to suppress the import error since the _version isn't generated unless the code is installed, e.g., on git clone.

@Saransh-cpp
Copy link
Member

I see, usually it is not recommended to import a package without installing it, but we can switch to try-except if that is a use-case. I believe contextlib was used to follow suggestions from ruff. No strong opinions on polluting the namespace, but usually it is okay to have such imports in __init__.py (and not include them in __all__).

@ntessore
Copy link
Collaborator Author

To be fair, it's been a while since I used package.__version__ now that importlib.metadata.version() is available out of the box. Perhaps the import isn't even necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants