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

Add an option to flag forbidden imports on the line at which they occur #411

Closed
wants to merge 16 commits into from

Conversation

AlexWaygood
Copy link
Collaborator

No description provided.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator Author

> ./stdlib/_collections_abc.pyi:4:23: NQA102 "# noqa: Y022,Y038,Y057" has no matching violations
> ./stdlib/_collections_abc.pyi:5:5: Y038 Use "from collections.abc import Set as AbstractSet" instead of "from typing import AbstractSet" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:6:5: Y022 Use "collections.abc.AsyncGenerator[YieldType, SendType]" instead of "typing.AsyncGenerator[YieldType, SendType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:7:5: Y022 Use "collections.abc.AsyncIterable[T]" instead of "typing.AsyncIterable[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:8:5: Y022 Use "collections.abc.AsyncIterator[T]" instead of "typing.AsyncIterator[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:9:5: Y022 Use "collections.abc.Awaitable[T]" instead of "typing.Awaitable[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:10:5: Y057 Do not use typing.ByteString, which has unclear semantics and is deprecated
> ./stdlib/_collections_abc.pyi:11:5: Y022 Use "collections.abc.Callable" instead of "typing.Callable" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:12:5: Y022 Use "collections.abc.Collection[T]" instead of "typing.Collection[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:13:5: Y022 Use "collections.abc.Container[T]" instead of "typing.Container[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:14:5: Y022 Use "collections.abc.Coroutine[YieldType, SendType, ReturnType]" instead of "typing.Coroutine[YieldType, SendType, ReturnType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:15:5: Y022 Use "collections.abc.Generator[YieldType, SendType, ReturnType]" instead of "typing.Generator[YieldType, SendType, ReturnType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:17:5: Y022 Use "collections.abc.Hashable" instead of "typing.Hashable" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:18:5: Y022 Use "collections.abc.ItemsView[KeyType, ValueType]" instead of "typing.ItemsView[KeyType, ValueType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:19:5: Y022 Use "collections.abc.Iterable[T]" instead of "typing.Iterable[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:20:5: Y022 Use "collections.abc.Iterator[T]" instead of "typing.Iterator[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:21:5: Y022 Use "collections.abc.KeysView[KeyType]" instead of "typing.KeysView[KeyType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:22:5: Y022 Use "collections.abc.Mapping[KeyType, ValueType]" instead of "typing.Mapping[KeyType, ValueType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:23:5: Y022 Use "collections.abc.MappingView" instead of "typing.MappingView" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:24:5: Y022 Use "collections.abc.MutableMapping[KeyType, ValueType]" instead of "typing.MutableMapping[KeyType, ValueType]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:25:5: Y022 Use "collections.abc.MutableSequence[T]" instead of "typing.MutableSequence[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:26:5: Y022 Use "collections.abc.MutableSet[T]" instead of "typing.MutableSet[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:28:5: Y022 Use "collections.abc.Reversible[T]" instead of "typing.Reversible[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:29:5: Y022 Use "collections.abc.Sequence[T]" instead of "typing.Sequence[T]" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:30:5: Y022 Use "collections.abc.Sized" instead of "typing.Sized" (PEP 585 syntax)
> ./stdlib/_collections_abc.pyi:32:5: Y022 Use "collections.abc.ValuesView[ValueType]" instead of "typing.ValuesView[ValueType]" (PEP 585 syntax)

We can probably avoid having to add a thousand million noqas to _collections_abc.pyi by add a per-file-ignore entry to our .flake8 file

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator Author

The isort thing is fixed now by using # isort: off, but there's still another issue: turns out ast.alias nodes don't have a lineno attribute on Python 3.8 and 3.9.

@JelleZijlstra
Copy link
Collaborator

Seems like there's a bigger problem: on 3.8 and 3.9 aliases don't have a lineno, so we can only do this on 3.10+ :(

That creates the problem that the location of # noqas will have to depend on the version of Python used to run flake8-pyi. Not a big problem for typeshed, but possibly annoying for some users.

Co-authored-by: Jelle Zijlstra <[email protected]>
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator Author

That creates the problem that the location of # noqas will have to depend on the version of Python used to run flake8-pyi. Not a big problem for typeshed, but possibly annoying for some users.

I'm not too worried about errors being reported on different lines for different versions. I feel like if we can give a more specific lineno for an error, we should, and I'm guessing most projects only run flake8 with a fairly recent Python version in CI -- so hopefully they won't be too annoyed with having errors reported on different linenos on different Python versions.

@AlexWaygood
Copy link
Collaborator Author

I am however not sure what a clean way to test this behaviour would be. Our test suite can't currently handle errors being reported on different lines, depending on which Python version you're using. I can think of various workarounds, but none that I like.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Collaborator Author

Following 17698fa, this PR no longer makes the plugin crash on py39, but the tests still fail due to the incorrect line numbers on py39.

@AlexWaygood
Copy link
Collaborator Author

@JelleZijlstra, thoughts on maybe gating this behaviour behind a flag? --precise-import-linenos? That would mean that users could stick with the current behaviour (consistent error reporting across Python versions), but also opt into more precise line numbers for their error codes if they wanted to. It would also solve the issue of how to test these error codes at flake8-pyi -- we could have a specific test file that enables that flag and checks that the errors are on the right line number when the flag is enabled.

@JelleZijlstra
Copy link
Collaborator

I'm not too fond of adding flags, it's generally better to have consistent behavior. It's definitely an option though.

We could probably handle this in the test suite by having separate test files per Python version.

@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood changed the title Flag forbidden imports on the line at which they occur Add an option to flag forbidden imports on the line at which they occur Jul 10, 2023
@github-actions
Copy link

This change has no effect on typeshed. 🤖🎉

@AlexWaygood
Copy link
Collaborator Author

I'm not too fond of adding flags, it's generally better to have consistent behavior.

In general I definitely agree. However, I think a flag is a good option here for two reasons:

  • I think it's nice for our default to be to have consistent behaviour across different Python versions. I think it could be confusing for users for us to say that we support Python 3.9, but then for them to find out that their noqa comments don't work if they actually run the plugin using Python 3.9, because the errors are now being flagged on different lines.
  • This change is arguably backwards incompatible, since users will have to change the location of their noqa comments. Making the new behaviour opt-in solves this problem.

@Akuli
Copy link
Collaborator

Akuli commented Jul 11, 2023

I think we should prefer consistent behavior, and it is less important what line number the error points at.

If we want to proceed with this PR, I'd suggest we make something hacky that gets the new behavior on Python 3.8 and 3.9, and we put it inside a sys.version_info check. We can delete the hack when we drop support for Python 3.8 and 3.9.

@AlexWaygood
Copy link
Collaborator Author

AlexWaygood commented Jul 11, 2023

If we want to proceed with this PR, I'd suggest we make something hacky that gets the new behavior on Python 3.8 and 3.9, and we put it inside a sys.version_info check.

I don't think it would be possible to do that on Python 3.9 in a reliable way without using the tokenizer, which I'm sorta reluctant to do since we currently only use the AST in flake8-pyi. So maybe we should just drop this PR for now, until we drop support for Python 3.9 (which will be a while off)? It's not really crucial to flag the bad import on the exact line it occurs on; just a nice-to-have, really.

@Akuli
Copy link
Collaborator

Akuli commented Jul 11, 2023

Agreed, maybe we should just do this later :)

@AlexWaygood
Copy link
Collaborator Author

I'll wait to see if Jelle has any ideas that I'm missing; if not, I'll just close this :)

@JelleZijlstra
Copy link
Collaborator

I don't have any better ideas than dropping support for 3.8 and 3.9, and it's a bit early for that.

@AlexWaygood
Copy link
Collaborator Author

I opened #413 so we remember to do this when we drop support for Python 3.9 :)

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.

3 participants