Skip to content

Conversation

@CAM-Gerlach
Copy link
Contributor

Fixes #1765

Feature Summary and Justification

The problem

Fixes the ability to statically type check PRAW's code that was apparently broken by #1676 and never caught, since there is apparently no continuous type checking. In summary, 37 files contain if TYPE_CHECKING blocks with imports of praw that only are considered statically by the type checker, which are relative imports that traverse outside the package. This causes mypy, stubgen, etc. runs to fail with a critical error, as traversal outside a Python package is not valid for relative imports.

The solution

Instead, the exact same intended effect (exposing the top level praw package as a valid name to the type checker) can be achieved by using a shorter absolute import (import praw), which has the added benefit of not breaking if the file is ever moved (as one apparently was). This has no effect at runtime or in any context but static type checking, given TYPE_CHECKING is always false otherwise and thus the blocks will not execute.

Additional tests performed

In addition to the automated tests with this PR, I also tested this locally by multiple means:

  • pre_push.py output no errors and made no modifications (static mode)
  • Building the docs worked with no errors
  • The docs were byte-by-byte identical before and after this PR, and displayed fine
  • MyPy worked following this PR, not failing with a critical error nor reporting missing import errors related to this PR
  • pip install -e . worked fine
  • Running a full suite of tests with my PRAW-powered application was fully successful with this PR branch
  • Running MyPy against my application with py.typed active in the root of the PRAW package worked with no errors or messages
  • Uninstalling my release copy of praw from the active env and repeating all of the above confirmed everything was working and there were no issues with pulling in the installed version

Future work

I was originally going to add a py.typed file in this PR per PEP 561 to enable type checking PRAW-using applications against the library, but I figure its best to leave that to a separate PR to keep this one's scope as narrow as possible. I also hope to clean up the remaining type errors in PRAW, add type hints to prawcore and (if desired) add type checking to the CIs and/or locally to catch errors and avoid future regressions like this one.

@LilSpazJoekp
Copy link
Member

Do you by chance know how we could use typing annotation strings that don't have to be fully qualified?

@CAM-Gerlach
Copy link
Contributor Author

@LilSpazJoekp I personally use absolute imports and fully-qualified names myself for their greater clarity and explicitness (except for using from imports from typing, typing_extensions and collections.abc, to follow standard convention, plus package-level constants and pathlib.Path), but as far as I'm aware, to use non-fully qualified type annotations (in 3.7+ they needn't be string literals with from __future__ import annotations), you would need to import the names you want with from under the if TYPE_CHECKING block, as I believe was done previously before PR #1676 . Otherwise, the runtime, type checker and humans have no way of determining what the names should resolve to.

@LilSpazJoekp
Copy link
Member

That's how it was before but Sphinx had issues with creating links for them.

@CAM-Gerlach
Copy link
Contributor Author

Hmm, I see. I haven't personally run into that particular problem, so I'm not really sure how to handle it other than the absolute import like you did, sorry. If I do, I'll let you know.

@LilSpazJoekp
Copy link
Member

Ah no worries! I'll get this merged.

@LilSpazJoekp LilSpazJoekp merged commit fdc8a29 into praw-dev:master Jul 19, 2021
@LilSpazJoekp
Copy link
Member

Thank you! 🎇

@CAM-Gerlach
Copy link
Contributor Author

Thanks @LilSpazJoekp !

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.

Typechecking appears broken due to off-by-one relative package imports

2 participants