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

Fix py.typed usage for namespace packages #4199

Closed

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Nov 26, 2024

  • Fix py.typed marker file placement to close The current py.typed setup does not seem ideal #3413:

    For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.

  • Add a test for py.typed files, verify the original reported issue is gone (Skipping analyzing "pymatgen.io.vasp.inputs": module is installed, but missing library stubs or py.typed marker [import])
  • Double check config of analysis / ext / io, are they meant to be namespace packages as well?

Looks like some sub-modules (analysis / ext / io) are implicit namespace packages as well (no __init__.py)? i.e. pymatgen is namespace package, so do pymatgen.ext/pymatgen.analysis/pymatgen.io. Not sure if this is intended, AFAIK sharing the pymatgen namespace is sufficient to support pymatgen-analysis-defects/analysis/diffusion, pymatgen-io-validation?

Also quote Anthony Sottile:

imo a namespace package is just always a bad idea -- you're better to use a plugin system -- here's an example of how to set one up: https://youtu.be/fY3Y_xPKWNA

>>> import pymatgen
>>> pymatgen
<module 'pymatgen' (namespace) from ['/home/yang/developer/pymatgen/src/pymatgen']>

>>> import pymatgen.io
>>> pymatgen.io
<module 'pymatgen.io' (namespace) from ['/home/yang/developer/pymatgen/src/pymatgen/io']>
>>> import pymatgen.ext
>>> pymatgen.ext
<module 'pymatgen.ext' (namespace) from ['/home/yang/developer/pymatgen/src/pymatgen/ext']>
>>> import pymatgen.analysis
>>> pymatgen.analysis
<module 'pymatgen.analysis' (namespace) from ['/home/yang/developer/pymatgen/src/pymatgen/analysis']>

https://github.com/materialsproject/pymatgen/tree/master/src/pymatgen/ext
https://github.com/materialsproject/pymatgen/tree/master/src/pymatgen/io
https://github.com/materialsproject/pymatgen/tree/master/src/pymatgen/analysis

for ext in ("py", "json*", "yaml", "csv"):
for filepath in glob(f"pymatgen/**/*.{ext}", recursive=True):
unix_path = filepath.replace("\\", "/")
if unix_path.endswith("dao.py"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe dao.py is already included in sdist and bdist (not just this branch, but from PyPI)

@DanielYang59 DanielYang59 deleted the 3413-py-typed-file branch December 1, 2024 06:59
@DanielYang59 DanielYang59 restored the 3413-py-typed-file branch December 1, 2024 07:25
@DanielYang59 DanielYang59 reopened this Dec 1, 2024
@DanielYang59
Copy link
Contributor Author

Still I would prefer closing this and let maintainers take over, esp. around the migration from namespace system to entry points.

@DanielYang59 DanielYang59 deleted the 3413-py-typed-file branch December 1, 2024 07:50
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.

The current py.typed setup does not seem ideal
1 participant