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

Don't forget to add py.typed file to package metadata #936

Merged
merged 5 commits into from
May 15, 2023

Conversation

sobolevn
Copy link
Contributor

While working on python/typeshed#10100 I've noticed that your project misses py.typed file in the package (version 2.1.0).

See:

» ls .venv/lib/python3.10/site-packages/invoke/
__pycache__/  __init__.py    config.py      executor.py  runners.py    watchers.py
completion/   __main__.py    context.py     loader.py    tasks.py
parser/       _version.py    env.py         main.py      terminals.py
vendor/       collection.py  exceptions.py  program.py   util.py

This line of code should package py.typed as well.
Can you please make 2.1.1 release? :)

@AlexWaygood
Copy link

@bitprophet, to clarify -- without this change, the py.typed file is not included in wheels uploaded to PyPI. Without the py.typed file, mypy doesn't know to look in site-packages/invoke/ for type hints, so will emit errors when users try to import invoke in their code.

@bitprophet
Copy link
Member

bitprophet commented May 5, 2023

Guessing this was an oversight in the typing PR. Can you clarify why this wants to go in package_data and not MANIFEST.in? If I go the latter route (which is normally how I get extra files added to the dists and I'd prefer to be consistent with that) I do now see the py.typed file (and it was definitely not in there beforehand, I checked):

diff --git a/MANIFEST.in b/MANIFEST.in
index ba413a3b..61fc0ece 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -5,6 +5,7 @@ recursive-include invoke/completion *
 recursive-include sites *
 recursive-exclude sites/*/_build *
 include dev-requirements.txt
+recursive-include * py.typed
 recursive-include tests *
 recursive-exclude * *.pyc *.pyo
 recursive-exclude **/__pycache__ *
» python setup.py bdist_wheel
<snip>
» unzip -l dist/invoke-2.1.1-py3-none-any.whl|grep typed
        0  05-02-2023 02:04   invoke/py.typed

@sobolevn
Copy link
Contributor Author

sobolevn commented May 5, 2023

Done!

@bitprophet bitprophet changed the base branch from main to 2.1 May 8, 2023 21:32
@bitprophet
Copy link
Member

Thanks! I just changed the base branch to the bugfix branch too.

Was going to put a changelog entry in and call it done, but realized: it'd be nice for however you noticed this, to be added to the install test step somehow - if we're gonna have typing support we should make sure it actually works!

Is there a nice concise reproduce case for what happens when py.typed isn't in the dist/wheel? (eg, some arbitrary module that imports Invoke, does its own typechecks, and then runs mypy - or something like that)

I'd probably need to add whatever that test is, to invocations (which happens to be what invoke uses to run most of its CI bits) so I don't think there's a useful test you can add at the unit/integration level, since those already have a source checkout locally. But giving me the repro case is fine :)

@bitprophet
Copy link
Member

bitprophet commented May 8, 2023

Oh Github. naturally, changing the base + adding a changelog entry == conflict. Let's see if their TTW tooling lets me resolve that conflict correctly or if I just need to make my own integration branch after all.

(I usually default to just cherry-picking folks' commits instead of trying to force the PR itself to end up 100% correct...figured I would try the 'github way' again. alas!)

EDIT: it appears to have worked! nice. being able to commit into other peoples' forks is extremely, extremely weird still.

@bitprophet
Copy link
Member

bitprophet commented May 8, 2023

This is ready for merge but I'd like us to figure out the test angle before I do. Thanks!

@AlexWaygood
Copy link

AlexWaygood commented May 10, 2023

Here's a possible way of testing this in a GitHub Actions workflow:

jobs:
  check-pep561-compliance:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
        with:
          path: invoke
      - uses: actions/setup-python@v4
        with:
          python-version: "3.11"
      - run: |
          pip install mypy
          pip install ./invoke
          mkdir newdir
          cd newdir
          mypy -c "import invoke"

@bitprophet
Copy link
Member

Thanks @AlexWaygood, confirmed that works pretty well to blow up when there's no py.typed.

@pyinvoke pyinvoke deleted a comment from AlexWaygood May 12, 2023
@bitprophet
Copy link
Member

Deleted some confusion. Still seeing weird behavior trying to prove/disprove this in my CI-oriented tasks, but I'll figure it out. Maybe not late in the day on a Friday though 😓

@bitprophet
Copy link
Member

Cache invalidation: one of the top two compsci problems strikes again. (A py.typed was being preserved in a build directory and making me incredibly confused as I bounced between 'in dists, not in dists, present in source, not present in source'.)

bitprophet added a commit to pyinvoke/invocations that referenced this pull request May 15, 2023
@bitprophet
Copy link
Member

@bitprophet
Copy link
Member

bitprophet commented May 15, 2023

Hm, doesn't look like faux-changing the base branch made GH do a rebase. I can 'rebase and merge' but I kinda wanted to 'rebase and prove the tests pass' first. Will try bouncing back and forth a bit (plus main branch now has my it's-gonna-break fix in it too.)

EDIT: nada. whatever. gonna just go back to my local integration branches in future I think 🙃 anyway, merging!

@bitprophet bitprophet changed the base branch from 2.1 to main May 15, 2023 22:01
@bitprophet bitprophet changed the base branch from main to 2.1 May 15, 2023 22:02
@bitprophet bitprophet merged commit 616b23a into pyinvoke:2.1 May 15, 2023
3 checks passed
@bitprophet
Copy link
Member

Invoke 2.1.2 out now! Thanks again.

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.

None yet

3 participants