+1 this would also align with the work Caitie is doing for adding scalafmt

renamed. thanks for bringing it up!

renamed. thanks!

corrected.

corrected.

corrected.

corrected.

good idea! moved.

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.

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...

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

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?

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?

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.

--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.

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

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.

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.

N/A anymore after moving the task to backend.

corrected.

implemented.

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

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

corrected.

updated.

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.

updated.

tests added for all cases mentioned.

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

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())

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

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

corrected. thanks!

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.

rephrased. thanks!

refactored.