-
-
contrib/python/src/python/pants/contrib/python/checks/register.py (Diff revision 1) 'Format targets.' doesn't seem like it'd be descriptive enough to a casual onlooker. what does it mean to "format a target" exactly?
how about 'Autoformat source code.' instead?
-
-
probably makes sense to default this to
./.isort.cfg
so in the general case folks don't need to understand where the configuration lives in the repo every time they run this? -
-
-
contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1) ws
-
testprojects/src/python/isort/python/.isort.cfg (Diff revision 1) 'Skip this directory under this directory'?
Introduce fmt goal, isort subgoal
Review Request #4134 — Created Aug. 2, 2016 and submitted
Information | |
---|---|
wisechengyi | |
pants | |
3747 | |
Reviewers | |
pants-reviews | |
benjyw, kwlzn, stuhood, zundel |
Introduce isort task, so users don't have to sort their python imports manually.
The idea of subgoal under
fmt
goal is that in the future, there can befmt.{go, java, mypi, etc}
Behavior:
1../pants fmt.isort <targets>
will sort the files only related to those targets, but the way of finding the config is vanilla.
2. Additional arguments can be passed as passthrough. e.g../pants fmt.isort <targets> -- --check-only
3../pants fmt.isort -- <args, "e.g. --recursive ." >
means both the files to be sorted and the way of finding the config are vanilla.
4../pants fmt.isort means ./pants fmt.isort ::
and NOT the entire repo directory which could include files not in any target.Other change:
1. Add a tip at pythonstyle to direct user to usefmt.isort
if there is issue related to import order.
2. Usebuild-support/bin/isort.sh
is changed fromisort <args>
to./pants fmt.isort -- <args>
https://travis-ci.org/pantsbuild/pants/builds/150804028
-
Any reason not to put this in the Python backend? It doesn't seem particularly risky or experimental, and it is widely useful.
-
build-support/bin/isort.sh (Diff revision 1) Add a comment somewhere in here specifying that we'll replace this script with an invocation of
./pants fmt
... -
contrib/python/src/python/pants/contrib/python/checks/BUILD (Diff revision 1) No need to repeat the target name when it's the same as the directory name.
-
-
discovered that
import isort
callsreload(sys)
causing side effects on Pants' exception handling. will shape it with binary util.
-
-
-
-
Perhaps it'd make sense for this to only apply to the same target types that python-eval is applied to.
-
contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1) Update name?
-
contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1) Could you add tests for
- missing config-file option
- what happens if there are no python containing targetsoptionally,
- what happens if there are no resorts necessary
- what the output looks like? If isort fails, what's that look like? -
contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1) I think it would be better to explicitly use a temporary workspace.
Change Summary:
almost there. still need to address nick's comment
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+280 -45) |
-
-
contrib/python/src/python/pants/contrib/python/isort/isort.py (Diff revision 2) I don't understand what this is for? Also, why it's in contrib.
Change Summary:
implemented the behaviors as discussed, and added more test coverage.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+285 -46) |
Change Summary:
typo fix
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+285 -46) |
-
-
src/python/pants/backend/python/register.py (Diff revision 4) hmm i guess this is probably not a good idea. any good way to get have
PythonTaskTestBase
recognizeresouces
created?
-
This is great - I love to see stuff like this pulled into a task.
-
src/python/pants/backend/python/tasks/python_isort.py (Diff revision 4) How long does this take to run? It only needs to sort changed files, so if this tasks takes a noticable amount of time you could consider hooking it up to the invalidation framework.
-
tests/python/pants_test/backend/python/tasks/test_python_isort.py (Diff revision 4) Errant comment, I think.
-
tests/python/pants_test/backend/python/tasks/test_python_isort.py (Diff revision 4) I think that the canonical way to make test targets is
create_file
and thenmake_target
now? I see some pertinent examples in the contrib/python plugin.
-
Two more remaining comments, otherwise LGTM!
-
src/python/pants/backend/python/tasks/python_isort.py (Diff revision 4) How about "Autoformats Python source files with isort."?
Then in
./pants fmt -h
, the fmt.isort section will sayfmt.isort options: Autoformats Python source files with isort. #....
-
src/python/pants/backend/python/tasks/python_isort.py (Diff revision 4) You could just pass this in execute. I think it would make the control flow graph easier to follow if both target collections were created there.
Change Summary:
address comments
Testing Done: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+285 -46) |
Change Summary:
reverting build-support/bin/isort.sh so i can commit. will put out another RB to change that.