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.

Sign up for GitHub

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

Jump to bottom

Add py.typed file to package for PEP-561 compliance #299

Closed
wants to merge 1 commit into from
Closed

Add py.typed file to package for PEP-561 compliance #299

wants to merge 1 commit into from

Conversation

jdufresne
Copy link
Member

Per PEP-561, packages that include type information that can be consumed
by other libraries should distribute a py.typed file. This tells mypy
and other tools to use type information shipped with the library.

This requires moving distro from a single module file to a package so
that it can ship data files.

For details on PEP-561, see:
https://www.python.org/dev/peps/pep-0561/

For details on mypy using py.typed, see:
https://mypy.readthedocs.io/en/stable/installed_packages.html#creating-pep-561-compatible-packages

@jdufresne
Add py.typed file to package for PEP-561 compliance
723e67e 
Per PEP-561, packages that include type information that can be consumed
by other libraries should distribute a py.typed file. This tells mypy
and other tools to use type information shipped with the library.

This requires moving distro from a single module file to a package so
that it can ship data files.

For details on PEP-561, see:
https://www.python.org/dev/peps/pep-0561/

For details on mypy using py.typed, see:
https://mypy.readthedocs.io/en/stable/installed_packages.html#creating-pep-561-compatible-packages
funkyfuture
funkyfuture reviewed Jul 7, 2021
View reviewed changes
Copy link
Contributor

@funkyfuture funkyfuture 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. it seems to me there's a little more inside that doesn't relate to PEP-561 directly.

distro/__main__.py
@@ -0,0 +1,4 @@
from . import main
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from . import main
from distro import main

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO the relative import is a better choice. It avoids conflicts with any other "distro" module/package in the Python path. I'd prefer to keep this as-is unless there is a strong reason against it.

distro/__init__.py
@@ -1365,7 +1365,3 @@ def main():
logger.info("Version: %s", distribution_version)
distribution_codename = dist.codename()
logger.info("Codename: %s", distribution_codename)

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this moved to a separate module? wouldn't it be better to define a "console script" entrypoint?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a console script. That hasn't changed.

It was moved to a module within a package so that additional "data" files can be installed alongside the package. That data file is the PEP-561 py.typed file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Further, using __main__.py allows the user to run python -m distro once installed. This was supported with the module version, so I carried over this feature.

@nir0s
Copy link
Collaborator

nir0s commented Jul 7, 2021

Thanks @jdufresne, though this means that distro.py will not be distributable in itself any longer. The idea is that you can inject distro.py to machines during e.g. provisioning, and simply run python distro.py -j to get the info, and maybe use jq to parse it.

I'd rather not break that. Thoughts?

@jdufresne
Copy link
Member Author

@nir0s I see your point and it is a good one.

It is a bit unfortunate that the module isn't PEP-561 compliant but not such a big deal. AFAICT, it can't be done with a pure module.

@jdufresne jdufresne closed this Jul 7, 2021
@jdufresne jdufresne deleted the py-typed branch July 7, 2021 23:37
@funkyfuture
Copy link
Contributor

then what about

  1. increasing the overhead to achieve both, alike
distro
  |- __init__.py  # imports public api from distro.distro.__all__
  |- __main__.py  # as proposed in this pr
  |- distro.py  # keeps the '__name__ == "__main__"' idiom, needs to define __all__
  |- py.typed  # as proposed in this pr
  1. documenting all four intended usages in the README

?!

@jdufresne
Copy link
Member Author

then what about

1. increasing the overhead to achieve both, alike

distro
  |- __init__.py  # imports public api from distro.distro.__all__
  |- __main__.py  # as proposed in this pr
  |- distro.py  # keeps the '__name__ == "__main__"' idiom, needs to define __all__
  |- py.typed  # as proposed in this pr

1. documenting all four intended usages in the README

?!

Thank you for this suggestion and sorry for taking so long to get back to it.

I've implemented this in #315 along with the documentation.

@jdufresne jdufresne mentioned this pull request Oct 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@funkyfuture funkyfuture funkyfuture left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdufresne @nir0s @funkyfuture