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

Issue #1942 remove linting error ignores #1964

Merged

Conversation

Elnaril
Copy link
Contributor

@Elnaril Elnaril commented Nov 19, 2020

What was wrong?

PR #1940 added the flake8 error code E252 to be ignored, which was a temporary solution to wait until all these errors were fixed in the codebase.

How was it fixed?

  • E252 has been removed from the ignore section of flake8 in tox.ini
  • All errors raised by tox were fixed by adding a space around operators where missing

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

…ection of tox.ini

start to add spaces around operators where errors are raised by tox/flake8
@Elnaril
Copy link
Contributor Author

Elnaril commented Dec 18, 2020

Hello @pipermerriam , @carver , @cburgdorf , @gsalgado , I'm not sure who I should ask for a review so I picked up the top 4 contributors ! 😅 😄 Would it be possible to have this PR reviewed ? 🙏

Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

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

Yup, looks great, thanks for the contribution!

Would you mind stripping out the W503 also? While you're working on that, it seems that the two commits can be squashed too. But if you prefer, I'm happy to squash-merge it for you after the W503 change.

tox.ini Outdated
@@ -10,9 +10,6 @@ envlist=
max-line-length= 100
exclude=
ignore =
# E252 missing whitespace around parameter equals
# Already used too much in the codebase at the point of introduction.
E252,
# W503 line break before binary operator
# It's either this or W504 (line break _after_ binary operator), pick your poison.
# W503 gets enabled when E252 gets ignored, see:
Copy link
Contributor

Choose a reason for hiding this comment

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

W503 was only here because of E252, so we can actually delete the W503 ignore now, also.

@Elnaril
Copy link
Contributor Author

Elnaril commented Jan 5, 2021

@carver thanks for the review ! :-)

W503 was only here because of E252, so we can actually delete the W503 ignore now, also.

Actually I tried it but I'm not sure that's possible because when I comment out this flag:
image

flake8 gets grumpy:
image

and if I add the line break after the operator, from:
image

to:
image

I get the W504:
image

@carver
Copy link
Contributor

carver commented Jan 5, 2021

Interesting! I wonder if the commented out lines look to flake8 like an empty list that overrides the default list, which causes the same problem as adding E252. Anyway, no need to hunt that down right now. I'll just squash-merge unless you prefer to squash it yourself.

@Elnaril
Copy link
Contributor Author

Elnaril commented Jan 5, 2021

Yep, I agree the behaviour seems weird ...

I'll just squash-merge unless you prefer to squash it yourself.

That's all good to me if you do it, thanks ! :-)

@Elnaril
Copy link
Contributor Author

Elnaril commented Jan 5, 2021

Actually, you may be right about the comment (or empty include) confusing flake8:

with that:

image

the linting is ok:
image

but with that:
image

the errors are back ...

@Elnaril
Copy link
Contributor Author

Elnaril commented Jan 5, 2021

Shall I remove the include= and commit ?

@carver
Copy link
Contributor

carver commented Jan 5, 2021

Hm, seems like that confusing error will just come back the next time someone wants to include an exception.

@carver
Copy link
Contributor

carver commented Jan 5, 2021

Maybe change the comment like:

- W503 gets enabled when E252 gets ignored, see:
+ W503 gets enabled when any "ignore =" config is added (even the empty one), see:

and then I'll squash-merge.

@Elnaril
Copy link
Contributor Author

Elnaril commented Jan 5, 2021

Change is done

@carver carver merged commit 16e69e0 into ethereum:master Jan 5, 2021
@pacrob pacrob mentioned this pull request Dec 20, 2023
3 tasks
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

2 participants