Page MenuHomePhabricator

add a linter for python imports
ClosedPublic

Authored by PiRK on Sep 15 2021, 16:32.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC6f244fbea854: add a linter for python imports
Summary

isort sorts imports by type (CONSTANT_VARIABLE, the CamelCaseClass, then functions_and_variables) and alphabetically, and automatically separated into sections (standard library, external library, internal import).

See D10200's description for the rationale regarding the choice of parameters
--profile=black --line-length=79.

Depends on D10200, D10201, D10202, D10203, D10204, D10205, D10206

Test Plan
sudo arc liberate
arc lint --everything

Impact on duration
before:

$ time arc lint --everything
...
real	1m5,412s
user	4m6,869s
sys	0m35,975s

after:

$ time arc lint --everything
...
real	1m8,443s
user	4m17,270s
sys	0m38,601s

Diff Detail

Repository
rABC Bitcoin ABC
Branch
isort
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 16808
Build 33465: Build Diff
Build 33464: arc lint + arc unit

Event Timeline

PiRK requested review of this revision.Sep 15 2021, 16:32

Can you please time before/after this linter ? External linters tend to be costly.

before:

$ time arc lint --everything
...
real	1m5,412s
user	4m6,869s
sys	0m35,975s

after:

$ time arc lint --everything
...
real	1m8,443s
user	4m17,270s
sys	0m38,601s
PiRK planned changes to this revision.Sep 16 2021, 08:55

Investigating how to tweak the parameters to avoid undoing existing multiline imports when they would be shorter than 80 characters on a single line.

Fabien requested changes to this revision.Sep 28 2021, 18:48

This is missing the installation for the CI, please fill up the contrib/utils/install-dependencies.sh script

CONTRIBUTING.md
156 ↗(On Diff #30175)

Can you also extend this to include isort ?

arcanist/linter/ISortLinter.php
13 ↗(On Diff #30175)

There is certainly a website or github for isort ?

17 ↗(On Diff #30175)

Sort your python imports

28 ↗(On Diff #30175)

If you're not adding options you can just remove this

40 ↗(On Diff #30175)

This one belongs to the mandatory flags, as the linter won't work at all without it

55 ↗(On Diff #30175)

This is unused, remove

68 ↗(On Diff #30175)

Actually you can give an installation instruction here, like pip install isort

This revision now requires changes to proceed.Sep 28 2021, 18:48

address review: add url, improve description and installation instruction, remove unused methods, set parameters in getMandatoryFlags

Fabien requested changes to this revision.Sep 29 2021, 08:06

Please address the remaining items before submitting for review

This revision now requires changes to proceed.Sep 29 2021, 08:06

I missed an item in CONTRIBUTING.md: include isort in the paragraph that explains how to install mypy with pip install on debian.

Fabien requested changes to this revision.Sep 29 2021, 08:55

Please address the remaining items before submitting for review

This revision now requires changes to proceed.Sep 29 2021, 08:55

Please address the remaining items before submitting for review

I don't understand what you mean.

You missed that part:

This is missing the installation for the CI, please fill up the contrib/utils/install-dependencies.sh script

I see. I saw only the inline comments. Sorry, I will pay closer attention next time.

make install-dependencies.sh install isort for the CI.

I chose to install the package via pip. This ensures that the latest is always installed. In some debian package repositories there are still some older (pre 5.0.0) versions.

Set the minimum isort version to 5.6.4, which seems to be a stable version used by debian. 5.0.0 produces different results wrt to newlines before the first import compared to the latest version (5.9.3), whereas 5.6.4 produces identical results when run on our code.

This revision is now accepted and ready to land.Sep 29 2021, 15:41
This revision was automatically updated to reflect the committed changes.