scripted diff: fix E275 lint

willcl-ark commented at 8:48 am on November 8, 2022: contributor

Fix E275 errors in lint-python.py with flake8 5.0.4

After updating my venv to Python 3.6.15 and reinstalling flake8 (at the current version, 5.0.4), it now throws quite a few E275 which are fixed with this scripted diff.

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

Duplicate of #26257.

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

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

DrahtBot commented at 9:17 am on November 8, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26359 (p2p, refactor: #23443 (Erlay support signaling) follow-ups by naumenkogs)
#26356 (test: Use consistent assert functions by NorrinRadd)
#26325 (rpc: Return accurate results for scanblocks by aureleoules)
#26257 (script, test: python linter fixups and updates by jonatack)
#26222 (Introduce field element and group element classes to test_framework/key.py by sipa)
#25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
#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.

willcl-ark commented at 9:19 am on November 8, 2022: contributor

First, sorry I missed duplicating #26257 (but I also think a scripted diff is the best way to make this change). @MarcoFalke which other whitespace did you have in mind?

IMO if we are using a specific linter then we should try to ensure those lints pass (or are disabled) and not more without adding additional lints for it. Otherwise if we change “all” whitespace today, we are likely to miss more of the same un-linted whitespace errors being introduced in the future.

maflcko commented at 9:30 am on November 8, 2022: member

scripted diff is the best way

The scripted diff as of now may modify out-of-git-tree files with the wildcard. You’d have to use git to get the file list. Also, it doesn’t pass CI as of now.

@MarcoFalke which other whitespace did you have in mind?

Anything you could come up with. Pretty sure there are other whitespace/formatting checks in flake. If not, you can take one from black as example. (Not saying you should do this, just presenting this as a reason against).

specific linter then we should try to ensure those lints pass

Yes, the only supported version is the one installed by CI. Obviously we can’t support future versions that may not have been released yet or even written.

errors being introduced in the future

Yes, so fixing this without enforcement seems doubly-bad. (Again, not saying you should do this, just presenting this as another reason against).

willcl-ark force-pushed on Nov 8, 2022

in test/functional/rpc_signrawtransactionwithkey.py:84 in b002f55b88 outdated

80@@ -81,7 +81,7 @@ def witness_script_test(self):
81         # Create a new P2SH-P2WSH 1-of-1 multisig address:
82         eckey = ECKey()
83         eckey.generate()
84-        embedded_privkey = bytes_to_wif(eckey.get_bytes())
85+        embedded_privkey = bytes_to_wif (eckey.get_bytes())

maflcko commented at 9:32 am on November 8, 2022:

Pretty sure this is wrong.

maflcko commented at 9:34 am on November 8, 2022: member

Taking a look at the number of conflict, see also my template answer:

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

Time spent on review
Accidental introduction of bugs
(Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

For more information about refactoring changes and stylistic cleanup, see

Generally, if the style is not mentioned nor enforced by the developer notes, we leave it up to the original author to pick whatever fits them best personally and then leave it that way until the line is touched for other reasons.

Let me know if you have any questions.

willcl-ark commented at 9:34 am on November 8, 2022: contributor

OK thanks.

Well, I’ll switch this to use git files and makes sure it passes CI and leave it as an option vs #26257.

in test/functional/feature_nulldummy.py:75 in b002f55b88 outdated

71@@ -72,7 +72,7 @@ def create_transaction(self, *, txid, input_details=None, addr, amount, privkey)
72     def run_test(self):
73         eckey = ECKey()
74         eckey.generate()
75-        self.privkey = bytes_to_wif(eckey.get_bytes())
76+        self.privkey = bytes_to_wif (eckey.get_bytes())

flack commented at 9:37 am on November 8, 2022:

these seem to be matched by accident by the if( regex

scripted diff: fix E275 lint

-BEGIN VERIFY SCRIPT-
git grep -l "\ assert(" -- '*.py' | grep -v "src/" | xargs sed -i 's/\ assert(/\ assert\ (/g'
git grep -l "\ not(" -- '*.py' | grep -v "src/" | xargs sed -i 's/\ not(/\ not\ (/g'
git grep -l "\ if(" -- '*.py' | grep -v "src/" | xargs sed -i 's/\ if(/\ if\ (/g'
-END VERIFY SCRIPT-

f7c632e0ba

willcl-ark force-pushed on Nov 8, 2022

willcl-ark commented at 10:13 am on November 8, 2022: contributor

Thank you for your contribution. While this stylistic change makes sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project.

OK. I will just apply the patch locally as I’m using newer flake8.

willcl-ark closed this on Nov 8, 2022

maflcko commented at 10:23 am on November 8, 2022: member

No objection, if you want to bump the CI version and add the rule to the excludes. Should be non-controversial.

di: update lint dependencies

flake8 to 5.0.4
mypy to 0.971
pyzmq to 24.0.1
vulture to 2.6

e425443dab

willcl-ark commented at 3:10 pm on November 8, 2022: contributor

OK I can re-open with a bump to the CI in case the scripted diff is preferred over #26257

willcl-ark reopened this on Nov 8, 2022

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

This seems less pythonic and at first glance I’m not sure why it would be a better solution than the PR with two ACKs. I do think it’s preferable to give review feedback than open competing PRs.

maflcko commented at 3:23 pm on November 8, 2022: member

I said “add the rule to the excludes” should be uncontroversial, not the scripted-diff, which is still broken in many ways, as I pointed out already.

willcl-ark commented at 3:25 pm on November 8, 2022: contributor

Ah OK I understand. I will review the PR from @jonatack which I agree, without the brackets, is more pythonic.

willcl-ark closed this on Nov 8, 2022

maflcko commented at 3:42 pm on November 8, 2022: member

E301, E302, E303, … and others aren’t enforced either, so why would E275 be important enough to enforce?

willcl-ark commented at 4:03 pm on November 8, 2022: contributor

When python was bumped to 3.6.15 I made a new venv, installed flake8 et. al. (with a simple pip install flake8 – not using the pinned versions from ci/lint/04_install.sh) and ran the linter, which resulted in the E275 errors that this change fixed, but no E301, E302, E303, … errors. So I suppose newer flake8 is checking E275 by default?

I’m not sure if we expect people to manually extract and locally install pinned versions from that ci script (it is linked from the readme), add a requirements.txt to test/, or leave the status quo. But I suspected I would not be the only person to encounter this (and had I used GitHub’s search function like a good netizen I would have seen I was correct without needing to open this conflicting PR!)

maflcko commented at 4:07 pm on November 8, 2022: member

Yes, it is mentioned in the readme. Improvements are welcome, if there are any.

bitcoin locked this on Nov 8, 2023

scripted diff: fix E275 lint #26468

Conflicts