script, test: python linter flake8 E275 fixup, update dependencies

jonatack commented at 11:37 am on October 5, 2022: contributor

It is helpful to be able to run the python linter locally to review PRs and check local diffs and work. Fix the errors raised by ./test/lint/lint-python.py when run locally with flake8 5.0.4, which enforces rule E275 more strictly than previous versions, and update our python linter CI dependencies.

stickies-v commented at 11:49 am on October 5, 2022: contributor

Approach ACK - this has been on my to-do list too. I prefer fixing the issue as opposed to dropping rules.

jonatack force-pushed on Oct 5, 2022

stickies-v approved

stickies-v commented at 2:12 pm on October 5, 2022: contributor

ACK 395519be6

Nice to have lint-python.py run without all those warnings. Non-controversial change imo, could have put a space instead of removing the parenthesis but this is more pythonic so I’m in favour. We do have instances of assert (...) left where the parentheses could be removed but it’s orthogonal and not worth the change on its own, imo.

theStack commented at 2:22 pm on October 5, 2022: contributor

Concept ACK

DrahtBot commented at 5:34 pm on October 5, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kristapsk
Concept ACK	theStack
Stale ACK	w0xlt, willcl-ark, stickies-v, brunoerg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26356 (test: Use consistent assert functions by NorrinRadd)
#26325 (rpc: Return accurate results for scanblocks by aureleoules)
#26222 (Introduce field element and group element classes to test_framework/key.py by sipa)
#24005 (test: add python implementation of Elligator swift by stratospher)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

brunoerg commented at 7:42 pm on October 5, 2022: contributor

Concept ACK

w0xlt approved

w0xlt commented at 1:57 pm on October 21, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26257/commits/395519be61090cd9e8ae1f6d48399a3fb99ca9ec

maflcko commented at 8:23 am on October 24, 2022: member

CI is red

maflcko commented at 8:35 am on October 24, 2022: member

Also, not sure if we should enforce this whitespace, given that no other whitespace is enforced.

jonatack force-pushed on Oct 26, 2022

jonatack commented at 5:47 pm on October 26, 2022: contributor

Rebased and updated with latest fixups (CI was green on the last push with 2 ACKs here, so I suppose it was restarted to verify with the latest merges).

jonatack commented at 5:53 pm on October 26, 2022: contributor

Also, not sure if we should enforce this

As mentioned in the pull description, another option would be to drop the E275 rule.

maflcko commented at 6:30 pm on October 26, 2022: member

CI is still red.

To me it seems odd to enforce this whitespace but no other. If you want to enforce whitespace, it might be best to discuss in a separate pull, and then for all whitespace, unless there is a reason to do it for only this one.

DrahtBot added the label Needs rebase on Nov 4, 2022

jonatack force-pushed on Nov 4, 2022

DrahtBot removed the label Needs rebase on Nov 4, 2022

jonatack commented at 10:27 pm on November 4, 2022: contributor

Rebased and CI is green. The intent is only to have our python linter pass on CI and locally with the current version; E275 seems to be part of current flake8 (https://www.flake8rules.com/rules/E275.html) unless we opt out of it. Like @stickies-v, I mildly prefer fixing the issue with more pythonic code rather than dropping rules.

fanquake commented at 8:58 am on November 8, 2022: member

~0

Is there a particular reason this change is broken into commits per-directory? Seems like an odd way to break it up. Could squash the 4, and use a commit message more explicit than “fixups” given there’s only a single change here?

fanquake commented at 9:01 am on November 8, 2022: member

See also https://github.com/bitcoin/bitcoin/pull/26468/commits/634cb384347463d33417ba7b43b189240c8883f1 from #26468, that uses a scripted-diff, which seems like the best way to do this.

jonatack commented at 3:21 pm on November 8, 2022: contributor

See also 634cb38 from #26468, that uses a scripted-diff, which seems like the best way to do this.

I find that version less pythonic and the other comments are nits. It would be nice to fix this and this has been open for a while.

fanquake commented at 3:46 pm on November 8, 2022: member

and the other comments are nits.

I don’t think suggesting the use of a commit message that clearly explains a change, rather than multiple vauge messages, is a nit.

The message from https://github.com/bitcoin/bitcoin/commit/634cb384347463d33417ba7b43b189240c8883f1: "Fix E275 errors in lint-python.py with flake8 5.0.4", explains what’s being done, and why. This is much more useful when compared to "X: python linter X fixups" repeated 4 times.

willcl-ark approved

willcl-ark commented at 9:28 pm on November 8, 2022: contributor

ACK fc3ca6f87f16448c4f4b9ecc3afa813f12868c18

Happy to re-review if you do change up the commit messages.

I do prefer the result of this PR over how my scripted diff turned it out (sorry again @jonatack for opening that dupe without searching first!)

stickies-v approved

stickies-v commented at 5:19 pm on November 10, 2022: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/fc3ca6f87f16448c4f4b9ecc3afa813f12868c18

However, I think fanquake’s suggestion to make the commit messages more helpful is on point, so I would support doing that first. I hereby commit to quickly re-ACK once that’s done (assuming no other big changes).

brunoerg approved

brunoerg commented at 5:31 pm on December 1, 2022: contributor

ACK fc3ca6f87f16448c4f4b9ecc3afa813f12868c18

Tested locally the versions modified in 04_install and reviewed the changes in the code, most are similar. I agree on squashing and making the message more helpful, happy to re-ack if you address it!

DrahtBot added the label Needs rebase on Dec 9, 2022

script, test: fix python linter E275 errors with flake8 5.0.4 459cb637ac

script: update python linter dependencies 1e5e87cec3

jonatack force-pushed on Jan 3, 2023

jonatack commented at 8:02 pm on January 3, 2023: contributor

Rebased and addressed feedback. Opened #26801 as an alternative. I mildly prefer this version for its more pythonic code.

jonatack renamed this:
~~script, test: python linter fixups and updates~~
script, test: python linter flake8 E275 fixup, update dependencies
on Jan 3, 2023

DrahtBot removed the label Needs rebase on Jan 3, 2023

kristapsk approved

kristapsk commented at 8:59 pm on January 3, 2023: contributor

ACK 1e5e87cec32260a6dfd03f11040aed8b7bb4064e

kristapsk commented at 8:59 pm on January 3, 2023: contributor

I mildly prefer this version for its more pythonic code.

I also prefer this over #26801.

maflcko merged this on Jan 3, 2023

maflcko closed this on Jan 3, 2023

jonatack deleted the branch on Jan 3, 2023

sidhujag referenced this in commit f2ce945cb7 on Jan 4, 2023

fanquake referenced this in commit 20991071a6 on Jan 10, 2023

bitcoin locked this on Jan 3, 2024

script, test: python linter flake8 E275 fixup, update dependencies #26257

Reviews

Conflicts