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

Are there plans to add type hints or library stubs for MyPy? #31

Closed
ginomempin opened this issue Feb 24, 2021 · 8 comments · Fixed by #34
Closed

Are there plans to add type hints or library stubs for MyPy? #31

ginomempin opened this issue Feb 24, 2021 · 8 comments · Fixed by #34
Labels
invalid This doesn't seem right wontfix This will not be worked on

Comments

@ginomempin
Copy link
Contributor

ginomempin commented Feb 24, 2021

I'm using your library right now in a FastAPI app, and since everything's heavily typed in FastAPI (and Pydantic), I have MyPy enabled and it's using quite a strict configuration. Right now, it's complaining about the imports from starlette_context:

Code:

from starlette_context import context
from starlette_context.plugins import Plugin, RequestIdPlugin

Error:

Skipping analyzing 'starlette_context': found module but no type hints or library stubs  [import]
Skipping analyzing 'starlette_context.plugins': found module but no type hints or library stubs  [import]
See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports

I can always just suppress the error with # type: ignore but I was wondering if you were planning on adding type hints or stub packages/files, as recommended by MyPy here: https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-type-hints-for-third-party-library. I think that would be a better approach.

In case there's no plan for this yet, what would you say to a PR to add stub files? Basically, I think just adding the appropriate .pyi files next to each .py file would satisfy MyPy.

@tomwojcik tomwojcik added enhancement New feature or request help wanted Extra attention is needed labels Feb 24, 2021
@tomwojcik
Copy link
Owner

tomwojcik commented Feb 24, 2021

I got so used to disabling stubs in mypy.ini that I haven't even thought about adding them.

[mypy-starlette-context.*]
ignore_missing_imports = True

It's a great idea to add them and I will be happy with your contribution.

I never looked into those things. Do you have to create stub files manually?

Stubgen does the thing. Cool!

@ginomempin
Copy link
Contributor Author

Stubgen does the thing. Cool!

Yeah, I tried with stubgen to auto-gen the .pyi files.
Still needs some manual updates/changes, but it's a good start.

@tomwojcik
Copy link
Owner

@ginomempin for the last few days I was reading about mypy, stubgen and I was testing it in a few test apps. My conclusions:

  • stubfiles are created only for versions below python3 and I plan to support only 3.7+ as that's really where asgi started
  • IDE gets confused by pyi files
  • my example app that uses starlette-context without pyi files hasn't had any problems with mypy as everything is static-typed anyway and that's what mypy is using by default

Can you please share your configuration? I don't think mypy should complain. Are you using mypy.ini for configuration?

@tomwojcik tomwojcik added invalid This doesn't seem right wontfix This will not be worked on and removed enhancement New feature or request help wanted Extra attention is needed labels Apr 9, 2021
@ginomempin
Copy link
Contributor Author

ginomempin commented Apr 11, 2021

@tomwojcik

I don't use a mypy.ini or any mypy-specific configuration file. I use VS Code and with mypy enabled.

Screen Shot 2021-04-11 at 13 44 22

Here are some files that reproduce the error:

main.py

from starlette.applications import Starlette
from starlette_context import context
from starlette_context.middleware import ContextMiddleware

app = Starlette(debug=True)

VS Code's settings.json

"python.linting.mypyPath": "/path/to/starlette-context-test/venv/bin/mypy",
"python.linting.mypyArgs": [
    "--warn-unused-configs",
    "--follow-imports=normal",
    "--show-error-codes"
],

VS Code's logs on running mypy on main.py

> ~/path/to/starlette-context-test/venv/bin/mypy --warn-unused-configs --follow-imports=normal --show-error-codes ~/path/to/starlette-context-test/main.py
cwd: ~/path/to/starlette-context-test
##########Linting Output - mypy##########
/path/to/starlette-context-test/main.py:4: error: Skipping analyzing 'starlette_context': found module but no type hints or library stubs  [import]
/path/to/starlette-context-test/main.py:4: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
/path/to/starlette-context-test/main.py:5: error: Skipping analyzing 'starlette_context.middleware': found module but no type hints or library stubs  [import]
Found 2 errors in 1 file (checked 1 source file)

Manually running mypy manually on main.py with default configs

$ mypy main.py
main.py:4: error: Skipping analyzing 'starlette_context': found module but no type hints or library stubs
main.py:4: note: See https://mypy.readthedocs.io/en/latest/running_mypy.html#missing-imports
main.py:5: error: Skipping analyzing 'starlette_context.middleware': found module but no type hints or library stubs
Found 2 errors in 1 file (checked 1 source file)

@ginomempin
Copy link
Contributor Author

ginomempin commented Apr 11, 2021

@tomwojcik

After re-reading MyPy's docs on Creating PEP 561 compatible packages, I realized that since everything on your package is already properly typed, it is only required that a py.typed file is exported:

PEP 561 describes three main ways to distribute type information:

  1. A package has inline type annotations in the Python implementation.
    ...

...packages that supply type information via type comments or annotations in the code should put a py.typed file in their package directory. For example, here is a typical directory structure:

setup.py
package_a/
    __init__.py
    lib.py
    py.typed

I retested based on my example above, and true enough, just exporting a py.typed file as part of the package installation will resolve all the MyPy errors I was previously getting. There is no need for stub files (.pyi) (especially if you are only targeting Python 3.7 and up).

$ ll venv/lib/python3.8/site-packages/starlette_context
total 24
-rw-r--r--   1 gino  gino   273B Apr 11 13:30 __init__.py
drwxr-xr-x   5 gino  gino   160B Apr 11 13:30 __pycache__/
-rw-r--r--   1 gino  gino   1.3K Apr 11 13:30 ctx.py
-rw-r--r--   1 gino  gino   237B Apr 11 13:30 header_keys.py
drwxr-xr-x   6 gino  gino   192B Apr 11 13:30 middleware/
drwxr-xr-x  11 gino  gino   352B Apr 11 13:30 plugins/
-rw-r--r--   1 gino  gino     0B Apr 11 13:30 py.typed     

@ginomempin
Copy link
Contributor Author

I submitted PR #36 to basically remove all the 14 .pyi files I added.
I am sorry for the trouble that caused. :(

It should be enough to keep the py.typed file though.

@tomwojcik
Copy link
Owner

No worries. We both learned something new :) Anyway, the way I understand it indeed only py.typed is needed but I have mypy pre-commit hook and it's not complaining. Also, I don't have any issues using it in another projects without this file. So maybe it all boils down to the mypy config?

Anyway, it should be working well for all cases now.

@tomwojcik
Copy link
Owner

released :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants