Page MenuHomePhabricator
Differential D10124

add a linter for python imports
ClosedPublic
Actions

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 created this revision.Sep 15 2021, 16:32
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 15 2021, 16:32
PiRK requested review of this revision.Sep 15 2021, 16:32
Harbormaster completed remote builds in B16661: Diff 29977.Sep 15 2021, 16:32
PiRK mentioned this in D10120: [test] Add test for getblockheader verboseness.Sep 15 2021, 17:06
Fabien added a subscriber: Fabien.Sep 15 2021, 19:32

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

PiRK added a comment.Sep 16 2021, 04:53

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 edited the test plan for this revision. (Show Details)Sep 16 2021, 04:55
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.

PiRK updated this revision to Diff 30175.Sep 27 2021, 08:59
PiRK edited the summary of this revision. (Show Details)
PiRK added parent revisions: D10206: sort python imports in remaining test_framework and misc files, D10200: sort python imports in abc* functional tests with isort, D10205: sort python imports in wallet_* functional tests, D10204: sort python imports in rpc_* functional tests, D10203: sort python imports in p2p_* functional tests, D10202: Sort imports in interface_*, mining_* and mempool_* functional tests, D10201: sort python imports in feature_* functional tests.

rebase onto D10200, D10201, D10202, D10203, D10204, D10205, D10206

Harbormaster completed remote builds in B16792: Diff 30175.Sep 27 2021, 08:59
PiRK edited the summary of this revision. (Show Details)Sep 28 2021, 11:33
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
PiRK updated this revision to Diff 30196.EditedSep 29 2021, 07:06

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

Harbormaster completed remote builds in B16805: Diff 30196.Sep 29 2021, 07:07
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
PiRK updated this revision to Diff 30200.Sep 29 2021, 08:38

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

Harbormaster completed remote builds in B16808: Diff 30200.Sep 29 2021, 08:39
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
PiRK added a comment.Sep 29 2021, 12:44
In D10124#229719, @Fabien wrote:

Please address the remaining items before submitting for review

I don't understand what you mean.

Fabien added a comment.Sep 29 2021, 13:07

You missed that part:

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

PiRK added a comment.Sep 29 2021, 13:10

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

PiRK updated this revision to Diff 30202.Sep 29 2021, 13:29

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.

Harbormaster completed remote builds in B16809: Diff 30202.Sep 29 2021, 13:30
PiRK updated this revision to Diff 30204.Sep 29 2021, 15:29

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.

Harbormaster completed remote builds in B16811: Diff 30204.Sep 29 2021, 15:30
Fabien accepted this revision.Sep 29 2021, 15:41
This revision is now accepted and ready to land.Sep 29 2021, 15:41
Closed by commit rABC6f244fbea854: add a linter for python imports (authored by PiRK). · Explain WhySep 29 2021, 15:49
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC6f244fbea854: add a linter for python imports.

Revision Contents
Changeset List

Diff 30200

View Options

.arclint

Loading...
View Options

CONTRIBUTING.md

Loading...
View Options

arcanist/.phutil_module_cache

Loading...
View Options

arcanist/__phutil_library_map__.php

Loading...
View Options

arcanist/linter/ISortLinter.php

Loading...