Page MenuHomePhabricator

sort python imports in abc* functional tests with isort
ClosedPublic

Authored by PiRK on Sep 27 2021, 08:46.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCb29d6ca9bf28: sort python imports in abc* functional tests with isort
Summary

This is preparation work to add a new linter that will enforce this new sorting.

The command used was:

isort --profile=black --line-length=79 test/functional/abc*

The rationale for --profile=black is that this generate the same style of multiline imports that we already use: one import per line with trailing commas, use parentheses, ensure newline before comments.

Also, black is increasingly popular as a python liner and we may want to use it instead of autopep8 some day in the future. So it is a good idea to stay compatible.

The rationale for --line-length=79 is that it is the standard for autopep8, and is more strict (hence compatible with) black which uses 88.

Note that it also orders the imports by type: CONSTANT_VARIABLE, the CamelCaseClass, then functions_and_variables.

It can also affect the number of blank lines after the imports: one line before simple variables, two lines before top-level classes and functions.

Test Plan

ninja check-functional

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sep 27 2021, 08:46
Fabien requested changes to this revision.Sep 27 2021, 19:11
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/abc-get-invalid-block.py
10 ↗(On Diff #30168)

I need to revise my alphabet, or the linter doesn't work

This revision now requires changes to proceed.Sep 27 2021, 19:11
PiRK requested review of this revision.Sep 28 2021, 07:28
PiRK added inline comments.
test/functional/abc-get-invalid-block.py
10 ↗(On Diff #30168)

I scratched my head at this as well. The explanation is that it puts CONSTANTS first, then Classes, then variable_or_functions.

https://pycqa.github.io/isort/docs/configuration/options.html#order-by-type

There is an option, if we don't want this behavior : --dont-order-by-type

Fabien requested changes to this revision.Sep 28 2021, 17:57
Fabien added inline comments.
test/functional/abc-sync-chain.py
19 ↗(On Diff #30168)

Where does this change comes from ?

This revision now requires changes to proceed.Sep 28 2021, 17:57
PiRK requested review of this revision.Sep 29 2021, 06:43
PiRK added inline comments.
test/functional/abc-sync-chain.py
19 ↗(On Diff #30168)

I checked that the change to the number of blank lines after the imports is compatible with flake8, autopep8 --aggressive and black. All 3 of these linters accept both one or two blank lines in this situation. This is compatible with PEP8 rules, which specify to use 2 blank lines before top-level classes or functions, but not necessarily for variables/constants.

isortis a bit more opinionated than the other linters on this matter, which is good because it enforces more consistency.

This revision is now accepted and ready to land.Sep 29 2021, 08:04