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

Flake8 config cleanup #4888

Merged
merged 7 commits into from Jun 8, 2021
Merged

Flake8 config cleanup #4888

merged 7 commits into from Jun 8, 2021

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Jun 8, 2021

This shrinks our flake8 error ignore config to a more reasonable level. Most error codes are irrelevant since they concern formatting which is handled by black, nowadays. However, there are two error codes in particular which triggered me doing this.

F841: local variable is assigned to but never used

This is an error code which detects assigned variables if they are unused. While this is usually not a big deal for tests (although not desired), this can be a frequent source for bugs in the actual code.

An example in this PR are changes in distributed/profile.py which likely indicates that we lost something at some point in time or distributed/scheduler.py l 3820ff which is definitely a bug (not fixed, yet)

We can still ignore this for tests, which is now reflected in the setup.cfg

F811 Redefinition of unused name from line n

This warning is raised if we are redefining symbols, e.g. functions or global variables, etc. Mostly, this is harmless. However, for test suites in particular this causes the problem of allowing duplicate test function names. I found 5-10 tests which were duplicated and therefore the first definition was ignored. These are typically cases where the definition of the two functions is hundreds or thousands of lines apart and it is impossible for reviewers to detect something like this. All instances where I found something like this, I simply appended a _2 to the function name or similar.

Enabling this error code in particular caused most of the change in this PR since every fixture import we were doing would trigger this, e.g. the following would raise a warning in line 3 because loop was redefined in the local scope although it was already defined in global scope but remains unused.

The easy solution to this is to use pytests fixture collection system via conftest.py which is why I added one import there to make all fixtures available to all functions. pytest will figure out the rest.

from distributed.utils_test import loop

def test_loop(loop):
    pass

@mrocklin
Copy link
Member

mrocklin commented Jun 8, 2021

+1

@fjetter
Copy link
Member Author

fjetter commented Jun 8, 2021

Test failure is an unrelated timeout in test_AllProgress_lost_key

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @fjetter!

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

3 participants