Introduce fmt goal, isort subgoal

Review Request #4134 — Created Aug. 3, 2016 and submitted

wisechengyi
pants
3747
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 be fmt.{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 use fmt.isort if there is issue related to import order.
2. Use build-support/bin/isort.sh is changed from isort <args> to ./pants fmt.isort -- <args>

https://travis-ci.org/pantsbuild/pants/builds/150804028

  • 0
  • 0
  • 8
  • 2
  • 10
Description From Last Updated
  1. 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.

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

    2. renamed. thanks for bringing it up!

  2. '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?

  3. nit: year here and elsewhere new -> 2016

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

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

  6. this should probably raise TaskError instead of directly exiting.

  7. 'Skip this directory under this directory'?

  8. 
      
  1. Any reason not to put this in the Python backend? It doesn't seem particularly risky or experimental, and it is widely useful.

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

    1. 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. 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. pants repo sort is not using fmt.isort via --fmt-isort-passthrough-args

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

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

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

  3. No need to repeat the target name when it's the same as the directory name.

    1. N/A anymore after moving the task to backend.

  4. This may as well be private: _calculate_sources().

  5. 
      
  1. Thanks for this change - it should be super-useful!

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

  2. 
      
  1. 
      
  2. it might make sense to have an option of --skip

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

    1. 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. on a second thought, it is good to explicitly filter out synthetic targets.

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

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

  3. 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. tests added for all cases mentioned.

  4. I think it would be better to explicitly use a temporary workspace.

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

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

    1. 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())
      
  3. 
      
  1. 
      
  2. 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 recognize resouces created?

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

  3. 
      
  1. This is great - I love to see stuff like this pulled into a task.

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

  3. Errant comment, I think.

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

  5. 
      
  1. Two more remaining comments, otherwise LGTM!

  2. 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.
    
    #....
    
  3. 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.

  4. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

9ccbabf1d636f88192db5f00b256e50fd7b61189

Loading...