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

FA102 should ignore quoted annotations #11397

Closed
inoa-jboliveira opened this issue May 13, 2024 · 5 comments · Fixed by #11414
Closed

FA102 should ignore quoted annotations #11397

inoa-jboliveira opened this issue May 13, 2024 · 5 comments · Fixed by #11414
Assignees
Labels
bug Something isn't working

Comments

@inoa-jboliveira
Copy link

inoa-jboliveira commented May 13, 2024

ruff 0.4.4 (3e8878a 2024-05-09)

We have an application that targets Python 3.9+, but instead of from __future__ import annotations on all files that is kinda ugly and unnecessary most of the time, whenever people need PEP 604 syntax they put the annotation within quotes which is effectively the same as the future import does except when someones accesses said annotations in run time (we don't).

See excerpt below:

setup\split_code_files.py:96:34: FA102 Missing `from __future__ import annotations`, but uses PEP 604 union
   |
94 |         self.nodes = self.get_ast_nodes()
95 |
96 |     def get_yaml_nodes(self) -> 'dict[str, str] | None':
   |                                  ^^^^^^^^^^^^^^^^^^^^^ FA102
97 |         try:
98 |             with open(self.yaml_file) as yaml_file:
   |
   = help: Add `from __future__ import annotations`

I believe FA102 should just ignore the annotation if it is within quotes. It should not mark this as violation because the code is indeed compatible with Python 3.9.

@charliermarsh charliermarsh added the bug Something isn't working label May 13, 2024
@charliermarsh
Copy link
Member

This sounds right to me.

@AlexWaygood
Copy link
Member

I'm not sure, this feels like intentional behaviour to me (#11414 (comment)) -- one of the motivating reasons for FA102 is to enable UP037 to automatically remove quotes from your annotations for you. You say you find the from __future__ import annotations import ugly -- I suppose it's a matter of opinion, but I find quoted annotations much more ugly personally :-)

@AlexWaygood
Copy link
Member

AlexWaygood commented May 13, 2024

@inoa-jboliveira, what's your reason for enabling FA102 in the first place if you find the from __future__ import annotations import ugly? The purpose of this check isn't to ensure that your code is compatible with Python 3.9

@inoa-jboliveira
Copy link
Author

Hi @AlexWaygood , thank you for the reply. I have some considerations

what's your reason for enabling FA102 in the first place if you find the from __future__ import annotations import ugly? The purpose of this check isn't to ensure that your code is compatible with Python 3.9

Of course the purpose is to check if the code is compatible older verions, it says so in the docs:

Checks for uses of PEP 585- and PEP 604-style type annotations in Python modules that lack the required from future import annotations import for compatibility with older Python versions.
https://docs.astral.sh/ruff/rules/future-required-type-annotation/

I want the check, I just don't want the proposed fix. For us is quite important if the code is invalid, but there are multiple ways to solve this problem.

I don't like UP037 because it is an opinonated fix that is meant for when a class references itself before it is fully created. There is a lot more depth around the annotation issues and "from future import annotations" have been postponed multiple times because people cannot agree yet on it. I think it might some day become the right approach, but until so I might not want to use it.

From python docs:

https://docs.python.org/3/library/__future__.html#id2
from future import annotations was previously scheduled to become mandatory in Python 3.10, but the Python Steering Council twice decided to delay the change (announcement for Python 3.10; announcement for Python 3.11). No final decision has been made yet. See also PEP 563 and PEP 649.

@AlexWaygood
Copy link
Member

Of course the purpose is to check if the code is compatible older verions, it says so in the docs:

yeah, sorry, I got quite mixed up here 🙃 -- #11414 (comment). I'll go ahead and blame it on some significant jetlag :(

charliermarsh added a commit that referenced this issue May 21, 2024
… type annotations (#11414)

## Summary

If an annotation won't be evaluated at runtime, we don't need to flag
`from __future__ import annotations` as required. This applies both to
quoted annotations and annotations outside of runtime-evaluated
positions, like:

```python
def main() -> None:
    a_list: list[str] | None = []
    a_list.append("hello")
```

Closes #11397.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants