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

Improve @overload detection #435

Merged
merged 2 commits into from
Mar 1, 2019
Merged

Improve @overload detection #435

merged 2 commits into from
Mar 1, 2019

Conversation

asottile
Copy link
Member

Resolves #434

@Zac-HD
Copy link
Member

Zac-HD commented Feb 28, 2019

Hurray!

This fixes another bug too: in the current released version, using a decorator named overload causes an internal error (because pyflakes thinks that it's typing.overload, and tries to access a nonexistent scope name). We first noticed this a while ago in Hypothesis' build, but it's reproducible by running pyflakes on the following file:

overload = lambda f: f  # or `def overload(arg): return arg

@overload
def func(arg): pass

def func(arg): return ...

Happily I've confirmed that this change fixes pyflakes for both the minimal case above (which might be worth adding as another test case for this PR?) and for Hypothesis - thanks @asottile 😍

@asottile
Copy link
Member Author

@Zac-HD 🎉 though it was probably #423 that fixed that one ~o/

@Zac-HD
Copy link
Member

Zac-HD commented Feb 28, 2019

Yeah, probably... still fixed though and still fixed by you 😁

@asottile
Copy link
Member Author

heh only fair -- I broke it in the first place /o\

@sigmavirus24 sigmavirus24 merged commit 232cb1d into PyCQA:master Mar 1, 2019
@asottile asottile deleted the any_decorator_typing_overload branch March 1, 2019 16:20
@kelseyfrancis
Copy link

Thanks for fixing this! Could you all please cut a pypi release that includes this fix?

tueda added a commit to tueda/formset that referenced this pull request Dec 15, 2019
Note that because of a bug fix in Python 3.8, the position of
`noqa: F811` for @overload has to be changed (see also
https://gitlab.com/pycqa/flake8/issues/583). So, if we want to pass
flake8 both with Python 3.7 and 3.8, we need to put `noqa: F811` for two
lines per @overload. This situation will be resolved by a new release of
pyflakes with improving @overload detection PyCQA/pyflakes#435.
tueda added a commit to tueda/donuts-python that referenced this pull request Dec 15, 2019
Note that because of a bug fix in Python 3.8 (see
https://gitlab.com/pycqa/flake8/issues/583) we need `noqa: F811` for two
lines per @overload if we want to pass flake8 both with Python 3.7 and
3.8. This situation will be resolved by a new release of pyflakes with
improving @overload detection PyCQA/pyflakes#435.
@omry
Copy link

omry commented Jan 4, 2020

When is this expected to make it to flake8?

@asottile
Copy link
Member Author

asottile commented Jan 4, 2020

no current plan, but you can read my thoughts here or as a temporary workaround install from git+https://github.com/pycqa/pyflakes@revision_here

@omry
Copy link

omry commented Jan 4, 2020

Looks like this issue still happens with master on python 3.8.

@asottile
Copy link
Member Author

asottile commented Jan 4, 2020

doesn't happen for me? try uninstalling pyflakes before installing from git (I suspect you're not using the actual git install)

@omry
Copy link

omry commented Jan 4, 2020

You are right, I had a system wide flake8 installed and bash already had it cached as flake8.

@omry
Copy link

omry commented Jan 5, 2020

@ asottile, unfortunately depending on a git version in setup.py prevents publishing a package.
would you be able to make a dev/pre release?

@asottile
Copy link
Member Author

asottile commented Jan 5, 2020

it's a bit unusual to depend on pyflakes in setup.py -- could you elaborate on the usecase?

(I also don't have pypi permission, I believe @bitglue manages that)

@omry
Copy link

omry commented Jan 5, 2020

Sure:
I am depending on flake for linting my code here.

Because of this overload bug, I am also depending on pyflake to get this bugfix.
This dependency (as well as the one with isort which is due to a similar problem) are preventing me from publishing the package as is (and I need to hack those out before publishing).

@asottile
Copy link
Member Author

asottile commented Jan 5, 2020

ah I see -- those dependencies aren't for your consumers so there isn't really a need to put them in setup.py -- maybe put them in your nox file instead?

@omry
Copy link

omry commented Jan 5, 2020

developers are also consumers.
I am using "pip install -e .[dev]" to setup the development environment.
I have git commit hooks that requires the proper version, It's not just a CI thing.

my noxfile is also settings up in the same way.

@asottile
Copy link
Member Author

asottile commented Jan 5, 2020

yeah I notice you're using pre-commit -- I'm the author (you've got it set up in a less-than-supported manner -- first you don't need .pre-commit-hooks.yaml that's for providing hooks, next you're using the escape hatch local + system hooks instead of versioning directly in .pre-commit-config.yaml) -- but that's a separate discussion entirely :)

when I said consumers above, I meant people that would install your package -- I understand you still want to have a developer workflow, I'm just suggesting that while setup.py does work for that, it isn't really the intended mechanism (whereas something more for your developer workflow such as nox / tox / poetry or even requirements-dev.txt is more properly suited for that)

@omry
Copy link

omry commented Jan 5, 2020

Thanks for the pointers! didn't realize you are the author of pre-commit hooks.
About that, I was following the instructions from Black, I was a bit confused about the purpose of the second file.
Looking now, looks like Black got better instructions. I update and clean up the mess.

@omry
Copy link

omry commented Jan 6, 2020

I fixed the pre-commit config for black to not use the local escape hatch, but I am not sure how to do it for flake8 given the need to install a specific revision of pyflakes.

@asottile
Copy link
Member Author

asottile commented Jan 6, 2020

something like this:

-   repo: https://gitlab.com/pycqa/flake8
    rev: 3.7.9
    hooks:
    -   id: flake8
        additional_dependencies: [-e, git+https://github.com/pycqa/pyflakes@...]

@omry
Copy link

omry commented Jan 6, 2020

Awesome, thanks!

LefterisJP added a commit to LefterisJP/rotkehlchen that referenced this pull request Mar 13, 2021
We had this TODO for a long time. That when
PyCQA/pyflakes#435 made it into a release we
could get rid of the noqa:F811 exceptions.

We have indeed upgraded pyflakes since then and now it's doable
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.

F811 (redefinition of unused variable) incorrectly triggered with @overload decorator
5 participants