lgtm - handful of comments.
I also wonder if we shouldn't call this goal "fmt" tho (like Stu initially suggested - prounounced "FUMPT"), to align with "gofmt", "scalafmt" etc. it's a thing, afaict.

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?

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   renamed. thanks!

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

nit: year here and elsewhere new -> 2016

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

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/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

might it make sense for this to refer to the fully scoped option name?

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

Show all issues

this should probably raise TaskError instead of directly exiting.

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1)

ws

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

testprojects/src/python/isort/python/.isort.cfg (Diff revision 1)

'Skip this directory under this directory'?

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

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)

Show all issues

Add a comment somewhere in here specifying that we'll replace this script with an invocation of ./pants fmt ...

1. 

   Yi Cheng Aug. 3, 2016, 5:41 p.m.

   There are still some subtle differences. E.g. invoking isort from cli will respect .isort.cfg in subdirs (hence testprojects/src/python/isort/python/.isort.cfg), but fmt.isort does not currently and applies --fmt-isort-config-file to any .py file it sees. There should be more work for fmt.isort to be more well rounded, but I'll leave it out of the current change scope.

2. 

   Benjy Weinberger Aug. 3, 2016, 9:05 p.m.

   In that case, --fmt-isort-config-file seems superfluous, and we should just always use the nearest .isort.cfg (ascending parent dirs until we find one).  It seems like a bad idea to have less functionality than the standalone isort tool, especially if it's functionality we already use in our own repo...

3. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   pants repo sort is not using fmt.isort via --fmt-isort-passthrough-args

4. 

   Benjy Weinberger Aug. 6, 2016, 3:36 a.m.

   What? I think we're not on the same page here. You're saying that pants can't use your new functionality on its own code because it relies on the ability to have per-directory .isort.cfg, and I'm saying that your new functionality should do the same. The current implementation has a double problem:
   A) It's not powerful enough, because it can't support per-directory .isort.cfg.
   
   B) It has unnecessary flexibility. Specifically, the --fmt-isort-config-file option seems like overkill. Is there currently any need for it? Why not always use .isort.cfg, and then it would be pretty easy to use a per-dir one too.
   See what I'm saying?

5. 

   Yi Cheng Aug. 6, 2016, 4 a.m.

   Sorry typo there. I meant to say "Pants repo sort is NOW using fmt.isort via --fmt-isort-passthrough-args"
   A) with --fmt-isort-passthrough-args, it will support per directory sorting
   
   B) --fmt-isort-settings-path (used to be --config-file) will default to $CWD/.isort.cfg, and that is used on target level.
   Anything else I am missing?

6. 

   Benjy Weinberger Aug. 6, 2016, 2:47 p.m.

   I still find this confusing. 
   As far as I can tell, vanilla isort behavior (if you use it directly, without pants) is to ascend directories until you find a .isort.cfg and then apply that to the file. That is, for A/B/C/D.py it will look for A/B/C/.isort.cfg, A/B/.isort.cfg etc. 
   Why shouldn't this task do exactly the same thing, always. Why do we need a --settings-path option at all, and why do we require use of --passthrough-args in order to get behavior that isort users would expect to be the default?
   Or am I misunderstanding how this works?
   Imagine an existing repo with existing .isort.cfg files, that now tries to use ./pants fmt with no arguments. The right thing should happen. And it sounds like that is not currently the case.

7. 

   Yi Cheng Aug. 6, 2016, 9:49 p.m.

   --settings-path helps to be explicit using a single config for specified targets, which is related to pants being intrinsically target oriented instead of directory oriented.
   
   say layout looks like:
   buildroot/src/python/project_a/a.py
   buildroot/src/python/project_b/b.py
   buildroot/tests/python/project_a/test_a.py
   buildroot/tests/python/project_b/test_b.py
   
   
   
   
   Vanilla behavior is likely fine for a single project, but in a big repo, it is possible for project_a and project_b to adopt different styles. If we use the vanilla behavior with buildroot/.isort.cfg, both project_a and project_b will be affected. Whereas with this RB, we can use ./pants fmt.isort --settings-path=project_b.cfg src/python/project_a tests/python/project_b.
   
   
   With vanilla behevior, user can copy a folder around then inadverdently cause the formatting to break because there is a .isort.cfg somewhere. Whereas we specify it as below, it will not cause side unexpected effects.
   
   
   [fmt.isort]
   settings.path= "%(buildroot)s/.isort.cfg"
   
   
   Regarding "./pants fmt with no arguments", we can default it to ./pants fmt :: which is the same behavior as ./pants list with no arguments.

8. 

   Benjy Weinberger Aug. 6, 2016, 11:02 p.m.

   First of all, to clear up an apparent miscommunication on my part, when I said "no arguments" I meant "no options". That is, I should be able to use isort idiomatically (by which I mean, using the per-dir .isort.cfg) without having to know about magic options settings.  But, yes, clearly ./pants fmt with no target arguments should operate on all targets.
   As for your explanations above, alas I have to strongly disagree.  The argument that in a big repo different projects can have different styles is an argument IN FAVOR of per-dir .isort.cfg, because it means that the style rules are carried around with the code that must conform to that style. In your RB the user has to manually know which config to apply to which targets, which is hopelessly confusing and error-prone.
   Instead, it's far better to have project_a's config live under project_a's topmost dir, and the same for project_b.  If there are multiple such dirs (say, src/ and test/) then one can be symlinked to the other. Git can store symlinks. In practice, if there is no single style in the repo, then different styles will almost certainly be confined to different subdirs. Otherwise programmers will find it impossible to track which style they're supposed to follow in a given file.  Having a clear rule that says 
   Assuming that directory hierarchies have semantic meaning (which they invariably do), copying folders around won't break formatting unless you've moved a dir from project_a into project_b, in which case you WANT isort to fail.  To say that pants is not "directory oriented" isn't quite accurate. Source code is definitely directory oriented (e.g., ./pants target path/to/dir:: has meaning). It's dependencies are not directory oriented, but we don't care about that here.
   So basically I still think this change introduces unnecessary complexity, while not providing the desired out-of-the-box experience.  I would still recommend getting rid of --settings-path entirely, at least until there's a proven need for it, and just implement the standard .isort.cfg behavior

9. 

   Yi Cheng Aug. 7, 2016, 1:01 a.m.

   That is reasonable, and --setting-path can be dropped.
   About passthru, are we on the same page?
   
   I'd propose having ./pants fmt.isort -- <some dir> equivalent to /usr/bin/isort <some dir>
   Otherwise, given
   buildroot/src/python/project_a/a.py <- belongs to target_name='project_a'
   buildroot/src/python/project_a/b.py <- belongs to target_name='project_a'
   buildroot/src/python/project_a/x.py <- belongs to target_name='resources'
   
   
   ./pants fmt.isort src/python/project_a may create confusions whether to sort all 3 files or just (a.py, b.py).
   To summarize:
   
   1. ./pants fmt.isort <targets> will sort the files only related to those targets, but the way of finding the config is vanilla.
   
   2. ./pants fmt.isort -- <dir> means both the files to be sorted and the way of finding the config are vanilla.
   
   3. ./pants fmt.isort means ./pants fmt.isort :: and NOT the entire repo directory which could include files not in any target.

10. 

    Benjy Weinberger Aug. 7, 2016, 1:05 a.m.

    Yes, thanks for bearing with me through this. I concur with all 3 points in your summary, and about passthru.  I like this because it's simple, idiomatic and super-useful.

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.

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   N/A anymore after moving the task to backend.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

This may as well be private: _calculate_sources().

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

Thanks for this change - it should be super-useful!

discovered that import isort calls reload(sys) causing side effects on Pants' exception handling. will shape it with binary util.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

Show all issues

it might make sense to have an option of --skip

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   implemented.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

Show all issues

i think you want to skip the generated python files here (synthetic targets).

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   unlikely an issue as it does not require gen step. and here is the implemented for python checkstyle which does not have so either. https://github.com/pantsbuild/pants/blob/c6924b59784b9993b4ba8f6eb2b55429f21b7298/contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/checker.py#L175-L178

2. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   on a second thought, it is good to explicitly filter out synthetic targets.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/BUILD (Diff revision 1)

nit: year

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   corrected.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

Show all issues

nit: update docstring here.

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   updated.

contrib/python/src/python/pants/contrib/python/checks/tasks/isort/python_isort.py (Diff revision 1)

Perhaps it'd make sense for this to only apply to the same target types that python-eval is applied to.

1. 

   Yi Cheng Aug. 8, 2016, 5:35 a.m.

   is_evalable in python-eval only applies to PythonLibrary, PythonBinary for some reason, hence added as separate method is_isortable to include PythonTests as well.

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1)

Update name?

1. 

   Yi Cheng Aug. 6, 2016, 3:31 a.m.

   updated.

contrib/python/tests/python/pants_test/contrib/python/checks/tasks/isort/test_format_isort.py (Diff revision 1)

Show all issues

Could you add tests for 

- missing config-file option

- what happens if there are no python containing targets
optionally,

- what happens if there are no resorts necessary

- what the output looks like? If isort fails, what's that look like?

1. 

   Yi Cheng Aug. 8, 2016, 5:35 a.m.

   tests added for all cases mentioned.

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.

1. 

   Yi Cheng Aug. 8, 2016, 5:35 a.m.

   adopted unit test instead, and all test content are specified in file.

contrib/python/src/python/pants/contrib/python/isort/isort.py (Diff revision 2)

Show all issues

I don't understand what this is for? Also, why it's in contrib.

1. 

   Yi Cheng Aug. 6, 2016, 9:49 p.m.

   It is in contrib because import isort has side effects (https://github.com/timothycrosley/isort/issues/456), so we have to build a binary then invoke it as a script.
   This came from vanilla isrot invocation. 
   
   [tw-mbp-yic pants (isort)]$ cat $(which isort)
   #!/opt/twitter/opt/python/bin/python2.7
   
   # -*- coding: utf-8 -*-
   import re
   import sys
   
   from isort.main import main
   
   if __name__ == '__main__':
       sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
       sys.exit(main())
   

src/python/pants/backend/python/register.py (Diff revision 4)

Show all issues

hmm i guess this is probably not a good idea. any good way to get have PythonTaskTestBase recognize resouces created?

1. 

   Yi Cheng Aug. 8, 2016, 7:29 a.m.

   the only usages are in tests, so should be okay.

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.

1. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   Since it supports passthru with vanilla isort behavior e.g. python files not in any target, invalidation is not very applicable. 
   I can't speak of the performance, but folks seem happy https://github.com/timothycrosley/isort/issues/337

tests/python/pants_test/backend/python/tasks/test_python_isort.py (Diff revision 4)

Errant comment, I think.

1. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   corrected. thanks!

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 then make_target now? I see some pertinent examples in the contrib/python plugin.

1. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   Used create_file. cannot use make_target because it does not actually create a BUILD file on disk, so when testing ./pants fmt.isort which is equivalent to ./pants fmt.isort ::, it will scan for targets like list does, so it will fail.

Two more remaining comments, otherwise LGTM!

src/python/pants/backend/python/tasks/python_isort.py (Diff revision 4)

Show all issues

How about "Autoformats Python source files with isort."?
Then in ./pants fmt -h, the fmt.isort section will say

fmt.isort options:
Autoformats Python source files with isort.

#....

1. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   rephrased. thanks!

src/python/pants/backend/python/tasks/python_isort.py (Diff revision 4)

Show all issues

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.

1. 

   Yi Cheng Aug. 9, 2016, 1:09 a.m.

   refactored.