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 flake8 #11676

Merged
merged 2 commits into from
May 10, 2024
Merged

Add flake8 #11676

merged 2 commits into from
May 10, 2024

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Mar 30, 2024

Basically create_baseline_stubs plus fixes plus low-hanging fruit.

@srittau
Copy link
Collaborator Author

srittau commented Mar 30, 2024

Useful for plugin development (e.g. flake8-pyi), also needed by stubs for flake8 plugins.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

flake8-pyi (https://github.com/PyCQA/flake8-pyi)
+ pyi.py:186: error: Signature of "annotationsFutureEnabled" incompatible with supertype "Checker"  [override]
+ pyi.py:186: note:      Superclass:
+ pyi.py:186: note:          bool
+ pyi.py:186: note:      Subclass:
+ pyi.py:186: note:          Literal[True]
+ pyi.py:2308: error: Cannot assign to a type  [misc]

@srittau
Copy link
Collaborator Author

srittau commented Mar 30, 2024

Regarding primer: The hit in line 186 is pyflakes-related, only showing up now, because the pyflakes types were "hidden" by flake8 stubs not being available. Probably just needs type updates in flake-pyi.

The hit in line 2308 is due to monkeypatching and will need a "# type: ignore".

@Akuli
Copy link
Collaborator

Akuli commented Mar 31, 2024

Do we really need flake8 stubs in typeshed? Flake8 already has type annotations, although they don't have a py.typed.

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 31, 2024

Flake8 already has type annotations, although they don't have a py.typed.

They're not going to add one — see PyCQA/flake8#1560.

But I'm also not sure about adding flake8 stubs to typeshed. Flake8-pyi needs a bunch of types from flake8's internals, but that's largely because we do a bunch of unauthorised monkey-patching in flake8-pyi. Plugins that adhere to flake8's contract shouldn't generally need to use many types from flake8's internals in their type annotations. (If they do, that should probably be grounds for asking the flake8 maintainers to reconsider their decision not to add a py.typed file.)

In hindsight, I sorta regret adding pyflakes stubs to typeshed a while back, for similar reasons.

@srittau
Copy link
Collaborator Author

srittau commented Mar 31, 2024

But I'm also not sure about adding flake8 stubs to typeshed. Flake8-pyi needs a bunch of types from flake8's internals, but that's largely because we do a bunch of unauthorised monkey-patching in flake8-pyi. Plugins that adhere to flake8's contract shouldn't generally need to use many types from flake8's internals in their type annotations.

By that logic, most of our stubs would be much smaller and tighter. Most of our stubs expose a lot of internal API. And flake8-pyi is a good use case for why it makes sense to have flake8 stubs that expose these internals. Other flake8 plugins will also need similar access. Also, even our own stubs need access to some flake8 APIs, e.g. OptionManager

@AlexWaygood
Copy link
Member

By that logic, most of our stubs would be much smaller and tighter. Most of our stubs expose a lot of internal API.

Agreed! I feel like I've argued several times before in this kind of situation, though, that we shouldn't necessarily add stubs for things that look like implementation details which aren't meant to be exposed, so I don't feel like my position is inconsistent here :-)

@Akuli
Copy link
Collaborator

Akuli commented Mar 31, 2024

There are many possible options, but they range from "ok but not great" to "bad".

  • Add stubs (this PR): duplicates information that is already in flake8 itself
  • Just create a py.typed file in CI before running type checking: clearly a hack
  • Leave untyped: this has been fine so far, but as primer shows, flake8-pyi also doesn't benefit from some other typings
  • Turn flake8-pyi into a standalone tool: we would duplicate much of what flake8 does, and we'd still need flake8 for some things
  • Just use ruff's flake8-pyi support: not easy to update for us
  • Fork ruff so we can easily edit its flake8-pyi parts: painful to maintain, not everyone knows rust

@AlexWaygood
Copy link
Member

AlexWaygood commented Mar 31, 2024

  • Just use ruff's flake8-pyi support: not easy to update for us
  • Fork ruff so we can easily edit its flake8-pyi parts: painful to maintain, not everyone knows rust

I recently started working full-time at Astral, so updating the flake8-pyi parts of Ruff should be somewhat easier than it was, if we do encounter issues. E.g. I'm in a very good position to make and review PRs to ruff if there's something that needs a quick fix.

But ruff's PYI rules are still a long way from parity with flake8-pyi, and I'd be unwilling for us to stop using flake8-pyi until we're actually at parity there. I also can't really prioritise working on the PYI rules at ruff right now (there's more systemic, longer-term things that I've been asked to work on).

So I think the first of these is something we could potentially consider in the future, as ruff's support for stubs continues to improve. But for now I'd rather not.

@JelleZijlstra
Copy link
Member

I'm happy to use Ruff for general linting, but I'd prefer to keep flake8-pyi or a similar tool around for typeshed's use, at least until Ruff gains plugin support. I think it is important for this project to be able to define its own linting rules in a way that is controllable by typeshed's maintainers. It's great that @AlexWaygood is working on Ruff now and can help make fixes, but in the long term we can't rely on typeshed's and Astral's priorities always being aligned.

That said, flake8-pyi isn't the only flake8 plugin that could benefit from these types, so we may want to add them regardless of the future of flake8-pyi. flake8 plugins are common enough that I think these types are useful, but we should try to focus on those parts of flake8 that are commonly useful to plugins, so we don't add too much maintenance cost.

@AlexWaygood
Copy link
Member

Fully agreed with your comments on wanting a tool that we can easily rework at a moment's notice, and can completely understand not wanting to rely on a third-party like Astral.

My central concern with this PR is still that I'm not sure how useful these types really are to plugins other than flake8-pyi that abide by flake8's contract rather than monkey-patching internals. And, if these types are useful to plugins that "behave properly", then that really feels like excellent grounds to go back to flake8 and ask again if they would consider adding a py.typed file. If they say "no" again, then that changes things, but if non-flake8-pyi plugins really need these types then I think we should at least ask, given that flake8 is full typed.

@srittau
Copy link
Collaborator Author

srittau commented Apr 7, 2024

As stated before, we already have stubs in typeshed that rely on flake8, especially the config interface. So far, our policy has always been to annotate the internals of libraries, not just the public API. I wasn't a huge fan of that policy from the beginning (although I can't quickly find the relevant discussions), but changing it is outside the scope of this PR, and would mean changing quite a bit of our tooling.

@AlexWaygood
Copy link
Member

As stated before, we already have stubs in typeshed that rely on flake8, especially the config interface.

Okay. But if multiple plugins need it, that sounds like a great reason to go back to the flake8 maintainers and present them with more evidence that adding a py.typed file would be beneficial to the community. Last time I opened an issue asking if they'd consider adding a py.typed file, I only came with flake8-pyi -- and that wasn't a good argument, because flake8-pyi breaks the contract for plugins by reaching into flake8's internals. If we come with evidence of other plugins that behave themselves more and would also benefit from having a py.typed file, I feel like that's a stronger argument.

But anyway, I don't think it'll be the end of the world or anything if we have stubs for flake8 in typeshed, especially given that we already have stubs for a bunch of flake8 plugins in typeshed. Feel free to proceed if you think these are going to be helpful to the community :-)

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed, didn't see any red flags.

@JelleZijlstra JelleZijlstra merged commit 27cad85 into python:main May 10, 2024
43 checks passed
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

4 participants