scripted diff: fix E275 lint #26468
scripted diff: fix E275 lint #26468
Conversation
|
Duplicate of #26257. |
|
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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. |
The scripted diff as of now may modify out-of-git-tree files with the wildcard. You'd have to use
Anything you could come up with. Pretty sure there are other whitespace/formatting checks in flake. If not, you can take one from
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.
Yes, so fixing this without enforcement seems doubly-bad. (Again, not saying you should do this, just presenting this as another reason against). |
634cb38 to
b002f55
Compare
There was a problem hiding this comment.
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
- https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring
- https://github.com/bitcoin/bitcoin/blob/master/.github/PULL_REQUEST_TEMPLATE.md
- #15465
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.
| @@ -81,7 +81,7 @@ def witness_script_test(self): | |||
| # Create a new P2SH-P2WSH 1-of-1 multisig address: | |||
| eckey = ECKey() | |||
| eckey.generate() | |||
| embedded_privkey = bytes_to_wif(eckey.get_bytes()) | |||
| embedded_privkey = bytes_to_wif (eckey.get_bytes()) | |||
|
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. |
test/functional/feature_nulldummy.py
Outdated
| @@ -72,7 +72,7 @@ def create_transaction(self, *, txid, input_details=None, addr, amount, privkey) | |||
| def run_test(self): | |||
| eckey = ECKey() | |||
| eckey.generate() | |||
| self.privkey = bytes_to_wif(eckey.get_bytes()) | |||
| self.privkey = bytes_to_wif (eckey.get_bytes()) | |||
There was a problem hiding this comment.
these seem to be matched by accident by the if( regex
-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-
b002f55 to
f7c632e
Compare
OK. I will just apply the patch locally as I'm using newer flake8. |
|
No objection, if you want to bump the CI version and add the rule to the excludes. Should be non-controversial. |
flake8 to 5.0.4 mypy to 0.971 pyzmq to 24.0.1 vulture to 2.6
|
OK I can re-open with a bump to the CI in case the scripted diff is preferred over #26257 |
|
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. |
|
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. |
|
Ah OK I understand. I will review the PR from @jonatack which I agree, without the brackets, is more pythonic. |
|
E301, E302, E303, ... and others aren't enforced either, so why would E275 be important enough to enforce? |
|
When python was bumped to 3.6.15 I made a new venv, installed flake8 et. al. (with a simple 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 |
|
Yes, it is mentioned in the readme. Improvements are welcome, if there are any. |
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.