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

refactor: Create a common utility for looking up fully qualified names #194

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Daverball
Copy link
Collaborator

This should make some checks a lot more robust and makes it easier to add similarly robust checks to library specific mixins.

This is also a precursor to using is_typing in the annotation visitor, so we can handle aliases and module level accesses of Literal and Annotated.

This should make some checks a lot more robust and makes it easier to
add similarly robust checks to library specific mixins.
@Daverball
Copy link
Collaborator Author

Technically this implementation will allow fewer if TYPE_CHECKING: cases, since we don't unilaterally accept a TYPE_CHECKING import from any module if it's aliased or accessed through the non-typing module, but this seems fine to me (we do however still accept unqualified TYPE_CHECKING for now). If people run into issues where they have vendored TYPE_CHECKING into a different module, we can add a setting to add additional fully qualified names that should be treated as TYPE_CHECKING.

Either way this should be treated as a breaking change, but it's probably a good time for it anyways, we can drop 3.8 support and add 3.13 support at the same time.

@Daverball
Copy link
Collaborator Author

I added some basic handling for full name matching in the presence of star imports. I'm not fully convinced it's worth the extra complication yet, especially since they're an anti pattern and star imports may already break some of the other rules in subtle ways, we certainly don't have good test coverage for them. So I might revert this last commit again.

@Daverball
Copy link
Collaborator Author

Looks like ruff's current semantic model doesn't handle star imports, although I imagine they will support them, once they switch to red-knot as the backend. So I'm still neutral on adding this extra complication.

@Daverball
Copy link
Collaborator Author

Ruff also has a setting to specify additional typing modules, so we could do the same in order to still support vendored typing constructs (e.g. within a local compat module through local.compat .compat and ..compat to support both absolute and relative imports).

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.

1 participant