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 inline ignore comment #622

Closed
Cielquan opened this issue Apr 5, 2022 · 10 comments
Closed

add inline ignore comment #622

Cielquan opened this issue Apr 5, 2022 · 10 comments

Comments

@Cielquan
Copy link

Cielquan commented Apr 5, 2022

I have a py3.10 code base and run pyupgrade via pre-commit.

In this code base I use pydantic models which use

standard library typing types as defined in PEP 484 to define complex objects.

link to docs

So I have to define a model like this (example from docs):

from typing import List

from pydantic import BaseModel


class Model(BaseModel):
    simple_list: list = None
    list_of_ints: List[int] = None

Which in turn gets modified by pyupgrade:

class Model(BaseModel):
    simple_list: list = None
-    list_of_ints: List[int] = None
+    list_of_ints: list[int] = None

And this results in a RuntimeError from pydantic.

To prevent this I currently use the workaround of skiping the model files whole which I luckily have in their own subpackage by ignoring them via the pre-commit config. But skiping them whole is a strong solution and I would rather skip single lines with something like a # noqa comment of some sort.

@MarcoGorelli
Copy link
Contributor

you can use keep-runtime-typing

@Cielquan
Copy link
Author

Cielquan commented Apr 5, 2022

The skip is only needed for the model classes. Other classes or functions would also be skipped with this flag then, right? I would have to give up on annotation upgrades for everything then, which I would like to avoid.

I unfortunately forgot while writing the OP that I also (will) have pydantic models outside of the models subpackage, which only holds DB models. The codebase is a fastAPI app. But the issue only occurred with the DB models till now. So even having 2 entries in pre-commit config (One for the model subpackage with the flag and one for the rest without it.) won't cut it unfortunately.

@Cielquan
Copy link
Author

Cielquan commented Apr 5, 2022

As a temporary better solution than full ignore I have this now:

  - repo: https://github.com/asottile/pyupgrade
    rev: e695ecd365119ab4e5463f6e49bea5f4b7ca786b # frozen: v2.31.0
    hooks:
      - id: pyupgrade
        name: pyupgrade (not models)
        args: ["--py310-plus"]
        exclude: .*\/models\/.*
      - id: pyupgrade
        name: pyupgrade (models)
        args: ["--py310-plus", "--keep-runtime-typing"]
        files: .*\/models\/.*

Which in turn results in:

$ pre-commit run pyupgrade --all-files
pyupgrade (not models)...................................................Passed
pyupgrade (models).......................................................Failed
- hook id: pyupgrade
- exit code: 1
- files were modified by this hook

Rewriting packages/swlp-api/app/models/user/user.py
Rewriting packages/swlp-api/app/models/user/user_role.py

So the --keep-runtime-typing flag does not apply correctly? Or is my config not right?

@asottile
Copy link
Owner

asottile commented Apr 5, 2022

show your error? I suspect you're using python<3.9 but you've told pyupgrade you only support python>=3.10

@asottile
Copy link
Owner

asottile commented Apr 5, 2022

but anyway -- this feature request will not be implemented -- there's already the --keep-runtime-typing option for broken things like pydantic

@asottile asottile closed this as completed Apr 5, 2022
@Cielquan
Copy link
Author

Cielquan commented Apr 5, 2022

show your error? I suspect you're using python<3.9 but you've told pyupgrade you only support python>=3.10

The package itself is set to python>=3.10 in its metadata.
But I did neither set language_version nor default_language_version in the pre-commit config. Will test this tomorrow.

but anyway -- this feature request will not be implemented -- there's already the --keep-runtime-typing option for broken things like pydantic

What a bummer, would have been nice to skip at individual loc.

@asottile
Copy link
Owner

asottile commented Apr 5, 2022

in the future please search the issue tracker -- #574

@Cielquan
Copy link
Author

Cielquan commented Apr 6, 2022

#623 (comment)

please continue the discussion in the original thread -- I don't think that matters, list[T] works fine for me in python3.10 with pydantic -- show your actual error

Well the issue was, that I created database models with sqlmodel (bridge between pydantic and sqlalchemy) and then alembic failed the automated database migration. After some further manual testing I found that the issue were some annotations list[T] and when I changed them to typing.List[T] the issue was gone.

Parallel to this github issue and #623 I further investigated the whole time and found out that it had something to do with T being "T". T must be a string because of circular imports and thus using a if tpying.TYPECHECKING: guard. I do not use from __future__ import annotations because there were other issues with it.

In the end I just found the underlying issue, which is already fixed and only needs to be merged: pydantic/pydantic#3681

@asottile
Copy link
Owner

asottile commented Apr 6, 2022

ok so as I suspected, not a pyupgrade bug but a bug in a third party tool -- if you would have led with the actual stacktrace you saw this would have been very obvious and would have saved us all time

in the future, please describe your actual problem rather than jumping to prescribing a solution -- I've put together a quick tutorial which outlines a strategy for making bug reports: https://youtu.be/ritp4gAqNMI

@antonagestam
Copy link

I thought I'd share here for anyone looking for workarounds (not a feature request 😉). I stumbled into the need of an ignore comment today because there's a bug in mypy where it won't work properly with builtin tuple. The workaround is to use typing.Tuple, but pyupgrade --py310-plus --keep-runtime-typing replaces that with tuple.

A workaround that both works with mypy and prevents pyupgrade from replacing it is to use string syntax with typing.TypeAlias. In my case I replaced this implicit type alias:

_Shape2D = _HasShape2D | tuple[int, int] | Fraction

With this explicit one:

_Shape2D: TypeAlias = "_HasShape2D | Tuple[int, int] | Fraction"

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

No branches or pull requests

4 participants