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

Adding flake8-future-annotations #3072

Closed
TylerYep opened this issue Feb 20, 2023 · 11 comments
Closed

Adding flake8-future-annotations #3072

TylerYep opened this issue Feb 20, 2023 · 11 comments
Labels
plugin Implementing a known but unsupported plugin

Comments

@TylerYep
Copy link
Contributor

TylerYep commented Feb 20, 2023

Hi, I authored the flake8-future-annotations plugin: https://github.com/TylerYep/flake8-future-annotations and I think it would be a good addition to ruff.

The goal of the lint rule is to verifies python 3.7+ files use from future import annotations if a type is used in the module that can be rewritten using PEP 563 e.g. typing.List, Dict, Optional, Union, etc.

It pairs especially well with pyupgrade with the --py37-plus flag or higher, since pyupgrade only replaces type annotations with the PEP 563 rules if from future import annotations is present, which I also see is included in ruff.

I plan to take a stab at implementing it (I see many of the necessary components are already tracked in ast.rs) in the following order:

  • Add FA100 (missing future annotations import in Python 3.7+)
  • Add an autofix for FA100 (adds missing future annotations import)
  • Add FA102 (uses simplified types like list or dict but missing future annotations import in Python 3.7 to 3.10)

All feedback is welcome, thanks!

@charliermarsh
Copy link
Member

Cool, thanks for writing this up! My only hesitation is around how this overlaps / interacts with some of the functionality that we already support -- e.g., right now, you can enforce __future__ annotation imports via required-imports. So if I understand correctly, these rules would be similar (and the autofix would be equivalent), with the distinction that they'd only trigger under certain conditions -- e.g., quoted annotations or simplified types, is that right?

@charliermarsh charliermarsh added the question Asking for support or clarification label Feb 21, 2023
@TylerYep
Copy link
Contributor Author

Yes, the lint rule was designed such that it only raises an error if the file benefits from from __future__ import annotations because it uses some typing import, but doesn't complain for files that don't need it. This prevents adding extra from __future__ import annotations imports that aren't actually needed.

@charliermarsh
Copy link
Member

Hey sorry -- lost track of this one. I try not to inject too much opinion into what gets included and excluded, but what do you see as the benefit of omitting future annotations from files that don't need it? To me it feels easier to either use them in all files for a given package (for consistency of semantics) or none at all.

@TylerYep
Copy link
Contributor Author

I'm currently type-checking a large monorepo, and to avoid making changes to all files at once, adding future annotations only to changed files is easier for incremental rollout and safety on legacy code. I'm not sure if you can achieve the same effect using isort's require_imports.

Honestly fine either way you decide here, I recognize that adding redundant plugins can add to the future maintainability of a project. But I am interested to hear what the broader criteria for including/excluding lint rules is; the flake8-future-annotations plugin has ~600k downloads now, so it is getting some use, but it is unclear to me if ruff just eats all tools to become the mega-tool or is more selective at redirecting functionality.

@charliermarsh
Copy link
Member

If you're still interested, I'd be happy to support you in implementing this. It does seem complementary to what we already have with require_imports.

A couple questions...

I'm currently type-checking a large monorepo, and to avoid making changes to all files at once, adding future annotations only to changed files is easier for incremental rollout and safety on legacy code.

What's the workflow typically look like here? As in, how do the rules help with incremental migrations?

Add FA100 (missing future annotations import in Python 3.7 to 3.10)

Does FA100 not go beyond 3.10?

@TylerYep
Copy link
Contributor Author

TylerYep commented Mar 6, 2023

What's the workflow typically look like here? As in, how do the rules help with incremental migrations?

One example is I want to upgrade types in certain parts of a project to modern PEP 585 syntax (e.g. list[int | None] instead of List[Optional[int]]) . To do so, I need to add from __future__ import annotations to those files and then run pyupgrade on it.

In ruff, the existing mechanism to do so would be to add the future annotations line to required-imports, however if my codebase has 1000 files, it would be unwise to add the import to all files in one go, especially if many of the files don't need the line added at all. The more minimal process is to enforce this import to one subfolder at a time, deploying changes and monitoring for errors, and trying to keep these commits as small as possible (minimize touched files and lines) to limit the blast radius.

Does FA100 not go beyond 3.10?

Fixed above to clarify the lint rule is currently applied for Python 3.7+, but the rules are not strictly necessary past Python 3.10 because set[int] works without from __future__ import annotations in Python 3.10+, and pyupgrade fixes those when --py310-plus is enabled. However as many projects support older versions the import is still recommended.

@charliermarsh
Copy link
Member

One example is I want to upgrade types in certain parts of a project to modern PEP 585 syntax (e.g. list[int | None] instead of List[Optional[int]]) . To do so, I need to add from future import annotations to those files and then run pyupgrade on it.

Yeah this makes sense. I was mostly wondering if flake8-future-annotations helps with that, since in that case, you still need to add from __future__ import annotations manually in order to trigger that flow, right?

Fixed above to clarify the lint rule is currently applied for Python 3.7+, but the rules are not strictly necessary past Python 3.10.

Oh right right. (I was mistakenly wondering if the 3.10 limit was based on the original timeline for stabilizing future annotations.)

@charliermarsh charliermarsh added plugin Implementing a known but unsupported plugin and removed question Asking for support or clarification labels Mar 6, 2023
@TylerYep
Copy link
Contributor Author

TylerYep commented Mar 6, 2023

Yeah this makes sense. I was mostly wondering if flake8-future-annotations helps with that, since in that case, you still need to add from future import annotations manually in order to trigger that flow, right?

Yes, because flake8-future-annotations only raises the lint error if a file uses an import like typing.List or typing.Optional without future annotations. On files that don't use this error, the future annotations import does not need to be added. SO presumably the ruff version of this lint would detect those typing.List imports and auto-add future annotations to only those files.

@Avasam
Copy link
Contributor

Avasam commented Apr 7, 2023

I skimmed through the conversation here, I wanted to suggest a standalone from __future__ import annotations rule as well.
My reasoning is builtin exclusion from stub files. Basically wrapping all this configuration into a single rule:

[tool.ruff.isort]
required-imports = ["from __future__ import annotations"]

[tool.ruff.per-file-ignores]
"*.pyi" = [
  "I002", # from __future__ import annotations
]

Even more so in the context of eventually having extendible configuration, I would prefer leaving "required-imports" empty in my base config and let per-project configuration override it.


As for TylerYep's request to only adding the line when needed. I'd consider it dead code. It's a good standard to always add it when devs have to manually track it. But if tooling is aware of when it's needed of not, then devs don't have to think about it or see it when it's not needed.

In other words: it's an unused import and imo should be treated as such.

@TylerYep TylerYep changed the title Adding flake8-future-annotatations Adding flake8-future-annotations Apr 8, 2023
Avasam added a commit to Toufool/AutoSplit that referenced this issue Apr 21, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
Avasam added a commit to Toufool/AutoSplit that referenced this issue May 23, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
Avasam added a commit to Toufool/AutoSplit that referenced this issue May 23, 2023
https://beta.ruff.rs/docs/
https://github.com/charliermarsh/ruff

Massively simplify configurations and speedup linting thanks to Ruff. Adds more autofixes too.
Using `pathlib` instead of `os.path` is the modern way to go. However fixing this could introduce a bug if not careful. So I'm leaving it for later.

Existing related Ruff requests (nothing here is a blocker, just future improvements):
- astral-sh/ruff#1256
- astral-sh/ruff#3011
- astral-sh/ruff#3072
- astral-sh/ruff#3910
- astral-sh/ruff#2419
- astral-sh/ruff#3115
- astral-sh/ruff#1904
@charliermarsh
Copy link
Member

I think this is good to go after #4702, apart from the autofix.

@Kludex
Copy link

Kludex commented Jan 3, 2025

Is the autofix work being tracked somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Implementing a known but unsupported plugin
Projects
None yet
Development

No branches or pull requests

4 participants