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

Merge extend-ignore and ignore values for flake8 #6660

Merged
merged 6 commits into from
Jul 1, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Jul 1, 2022

Merges ignore and extend-ignore config options for flake8, as extend-ignore makes little sense when ignore is also configured and therefore overriding defaults.

This PR

  • Tests added / passed
  • Passes pre-commit run --all-files

@hendrikmakait hendrikmakait changed the title Merge extend-ignore and ignore values for flake8 Merge extend-ignore and ignore values for flake8 Jul 1, 2022
@hendrikmakait hendrikmakait requested a review from crusaderky July 1, 2022 12:06
@@ -3,14 +3,16 @@
# https://flake8.readthedocs.io/en/latest/user/configuration.html
# https://flake8.readthedocs.io/en/latest/user/error-codes.html

# Aligned with black https://github.com/psf/black/blob/main/.flake8
extend-ignore = E203, E266, E501
Copy link
Member

Choose a reason for hiding this comment

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

I think It's better to keep extend-ignore and remove the regular ignore

Copy link
Member Author

@hendrikmakait hendrikmakait Jul 1, 2022

Choose a reason for hiding this comment

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

I would be fine with this, one benefit I saw from keeping the ignore list and moving things there is that the default rules for ignore may change from version to version. By maintaining ignore, which currently maps to the defaults, we can effectively pin these.

If we assume that whatever rules flake8 selects as defaults are probably a good idea, I'd be more than happy to keep this rule-set even smaller. Since we pin the version of flake8 itself, this should not result in any surprises outside of a flake8 update.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine either way. FWIW, I am usually an advocate for strict linting. I trust the two of you to find a good configuration

Copy link
Member Author

@hendrikmakait hendrikmakait Jul 1, 2022

Choose a reason for hiding this comment

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

Not sure where I got my earlier defaults for flake8 from, but these are the actual ones: ['E121', 'W504', 'E123', 'W503', 'E24', 'E126', 'E226', 'E704']. This means that a bunch of rules have changed. For comparison, black uses ignore = E203, E266, E501, W503. Given our previous selection, this feels like a reasonably strict set of rules and would mean that we mainly have to start fixing import formatting E4**.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should sprinkle E402 in there, it's the only rule that's failing a lot due to

pa = pytest.importorskip("pyarrow")
and similar imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

Silenced E402 for tests.

@hendrikmakait hendrikmakait requested a review from fjetter July 1, 2022 14:41
@hendrikmakait
Copy link
Member Author

@fjetter: I have made significant changes since your last review. Could you give it another look please?

@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   10h 6m 47s ⏱️ + 27m 44s
  2 907 tests ±0    2 821 ✔️  - 2    82 💤 ±0  4 +2 
21 525 runs  ±0  20 570 ✔️ +1  950 💤  - 2  5 +1 

For more details on these failures, see this check.

Results for commit 0b377d0. ± Comparison against base commit 1bfd99c.

@fjetter fjetter merged commit 18a4642 into dask:main Jul 1, 2022
# Ignores below are aligned with black https://github.com/psf/black/blob/main/.flake8
E203, # Whitespace before ':'
E266, # Too many leading '#' for block comment
E501, # Line too long
Copy link
Member

Choose a reason for hiding this comment

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

Black ignores this because they use a different error code for line too long, we shouldn't ignore it

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.

3 participants