Skip to content

W504 - line break after binary operator #1660

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

W504 - line break after binary operator #1660

wants to merge 1 commit into from

Conversation

@guedou
Copy link
Member

@guedou guedou commented Oct 24, 2018 •
edited
Loading

This PR fixes #1659. I tried to limit the use of noqa but it is needed in some places.

@guedou guedou added the PEPin Apply PEP08 rules label Oct 24, 2018
@guedou guedou mentioned this pull request Oct 25, 2018
Merged
@codecov-io
Copy link

codecov-io commented Oct 25, 2018 •
edited by codecov bot
Loading

Codecov Report

Merging #1660 into master will decrease coverage by 0.51%.
The diff coverage is 79.9%.

@@            Coverage Diff             @@
##           master    #1660      +/-   ##
==========================================
- Coverage    85.6%   85.08%   -0.52%     
==========================================
  Files         181      180       -1     
  Lines       42218    41473     -745     
==========================================
- Hits        36140    35288     -852     
- Misses       6078     6185     +107
Impacted Files Coverage Δ
scapy/contrib/diameter.py 99.57% <ø> (ø) ⬆️
scapy/contrib/bgp.py 91.76% <ø> (ø) ⬆️
scapy/layers/dot11.py 90.81% <ø> (ø) ⬆️
scapy/contrib/mqtt.py 96.96% <ø> (ø) ⬆️
scapy/layers/tftp.py 37.13% <0%> (ø) ⬆️
scapy/layers/tls/crypto/groups.py 88.76% <0%> (ø) ⬆️
scapy/layers/tls/extensions.py 79.22% <0%> (-0.57%) ⬇️
scapy/contrib/tacacs.py 92.53% <0%> (ø) ⬆️
scapy/contrib/gtp.py 88.81% <0%> (ø) ⬆️
scapy/layers/dot15d4.py 71.66% <0%> (-0.41%) ⬇️
... and 65 more

@guedou guedou force-pushed the PEPin_W504 branch 14 times, most recently from 7d98e26 to 3d55979 Compare October 31, 2018 10:43
@guedou
Copy link
Member Author

guedou commented Oct 31, 2018

@gpotter2 @p-l- ready to be reviewed!

p-l-
p-l- reviewed Oct 31, 2018
View reviewed changes
Copy link
Member

@p-l- p-l- left a comment

Choose a reason for hiding this comment

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

Is it worth it? Maybe we could decide we ignore W504 globally?

gpotter2
gpotter2 reviewed Oct 31, 2018
View reviewed changes
@p-l-
Copy link
Member

p-l- commented Oct 31, 2018

(copy & paste of my review message here)

Is it worth it? Maybe we could decide we ignore W504 globally?

@guedou
Copy link
Member Author

guedou commented Oct 31, 2018

@p-l- most of the W504 errors were easily fixed. It was a lot of work because the code has many of them, but it seems OK to enforce W504.

@guedou
Copy link
Member Author

guedou commented Nov 2, 2018

@gpotter2 do you understand why the windows tests fail?

@guedou guedou force-pushed the PEPin_W504 branch 2 times, most recently from e9d542b to adb73f1 Compare November 30, 2018 14:37
@guedou guedou force-pushed the PEPin_W504 branch 2 times, most recently from 9d01980 to b1d0f83 Compare January 11, 2019 09:03
@gpotter2
Copy link
Member

gpotter2 commented Jan 16, 2019

I don’t see tests failing as abruptly:/ I’ll try to figure it out

@guedou
Copy link
Member Author

guedou commented Jan 16, 2019

Is it also failing on a real Windows system?

@gpotter2 gpotter2 mentioned this pull request Jan 17, 2019
Merged
p-l-
p-l- reviewed Jan 19, 2019
View reviewed changes
scapy/arch/bpf/core.py Outdated
Copy link
Member

@p-l- p-l- Jan 19, 2019

Choose a reason for hiding this comment

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

Could you move the if to a next line and remove at least the E501 new noqa?

Copy link
Member Author

@guedou guedou Jan 22, 2019

Choose a reason for hiding this comment

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

rewritten with in instead of find() to save space.

p-l-
p-l- reviewed Jan 19, 2019
View reviewed changes
scapy/arch/windows/__init__.py Outdated
Copy link
Member

@p-l- p-l- Jan 19, 2019

Choose a reason for hiding this comment

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

I don't understand this change. Wouldn't it be cleaner to keep the previous version and cut the longest lines to remove the noqa?

Copy link
Member Author

@guedou guedou Jan 22, 2019

Choose a reason for hiding this comment

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

noqa removed

Copy link
Member

@p-l- p-l- Jan 25, 2019

Choose a reason for hiding this comment

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

I still don't understand. error_msg += ... creates a new string each time.

@p-l-
Copy link
Member

p-l- commented Jan 25, 2019

@p-l- most of the W504 errors were easily fixed. It was a lot of work because the code has many of them, but it seems OK to enforce W504.

I'm not sure to agree. IMO W504 should be permitted. They are disabled by default, and I'm not sure it makes sens to forbid both W503 and W504.

@guedou
Copy link
Member Author

guedou commented Jan 29, 2019 via email

@guedou guedou closed this Jan 30, 2019
This was referenced Feb 7, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@p-l- p-l- p-l- left review comments

@gpotter2 gpotter2 gpotter2 left review comments

Assignees

No one assigned

Labels

PEPin Apply PEP08 rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Travis] flake8 was updated to 3.6.0

4 participants

@guedou @codecov-io @p-l- @gpotter2