Skip to content

Enable flake8 rule E225 which checks for missing whitespace around op…#14534

Closed
jbampton wants to merge 1 commit intobitcoin:masterfrom
jbampton:flake8-fix-E225
Closed

Enable flake8 rule E225 which checks for missing whitespace around op…#14534
jbampton wants to merge 1 commit intobitcoin:masterfrom
jbampton:flake8-fix-E225

Conversation

@jbampton
Copy link
Contributor

…erators

Lint Python code for rule E225

@fanquake fanquake added the Tests label Oct 20, 2018
@practicalswift
Copy link
Contributor

ACK 74f4e2d648eb40dd2315b46d87d6824f9f419e56 assuming nit is addressed and squash :-)

@jbampton
Copy link
Contributor Author

Hey @practicalswift the nit is fixed and now squashed 👍

@DrahtBot
Copy link
Contributor

Reviewers, 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.

@practicalswift
Copy link
Contributor

practicalswift commented Oct 21, 2018

ACK 0ad199b

Nice to never have to think about or comment on this ever again in future code reviews thanks to this check: review automation FTW :-)

@Empact
Copy link
Contributor

Empact commented Oct 21, 2018

utACK 0ad199b changes are whitespace only (https://github.com/bitcoin/bitcoin/pull/14534/files?w=1), the result is easier to read and the practice is automatically enforced

@ken2812221
Copy link
Contributor

It would be better to use scripted-diff for this kind of change. Use autopep8 or something else can auto format.

@jbampton
Copy link
Contributor Author

Hey everyone, I did use autopep8 for this.

From the project root directory the command I ran was:

autopep8 --in-place --select=E225 -r .

I would be happy to add other flake8 checks if you want to this pull request.

autopep8 uses pycodestyle

https://pep8.readthedocs.io/en/latest/intro.html#error-codes

Here is the link to autopep8:

https://github.com/hhatto/autopep8

@ken2812221
Copy link
Contributor

What I mean is writing the commands you were using into the commit message by scripted-diff. Then travis-ci will run the commands again to check if it is the same. So reviewers won't have to run the commands manually on their own computer to check if the change is correct or not.

For example (Haven't tested):

-BEGIN VERIFY SCRIPT-
pip3 install --user autopep8
autopep8 --in-place --select=E225 -r .
-END VERIFY SCRIPT-

@jbampton
Copy link
Contributor Author

Hey @ken2812221 thanks for explaining the process further.

Does that mean you want me to commit again with a scripted diff ?

@promag
Copy link
Contributor

promag commented Oct 22, 2018

How can we ensure other pulls don't break master if merged after this?

+1 scripted diff.

Edit: @ken2812221 install in the script?

@ken2812221
Copy link
Contributor

ken2812221 commented Oct 23, 2018

@jbampton You can git commit --amend to rewrite the commit message and force push it.

@promag Yes, I assume travis does not have autopep8 installed. Also we don't want to change travis script. I think this is the only way to install it.

@maflcko
Copy link
Member

maflcko commented Oct 23, 2018

Both variants are allowed in python and the whitespace is not really "missing". Tend to NACK, as I don't see the advantage of doing this code churn for everyone.

@maflcko
Copy link
Member

maflcko commented Oct 23, 2018

See also #14540

@DrahtBot
Copy link
Contributor

Needs rebase

@maflcko
Copy link
Member

maflcko commented Oct 26, 2018

Trivial pull request still needs rebase. Closing for now.

@maflcko maflcko closed this Oct 26, 2018
@jbampton jbampton deleted the flake8-fix-E225 branch October 14, 2019 04:13
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants