Change default ./pants fmt.isort <empty> behavior to no-op; Add sources check for isort.

Review Request #4327 — Created Oct. 19, 2016 and submitted

wisechengyi
pants
3989
pants-reviews
benjyw, jsirois, kwlzn, mateor, nhoward_tw, stuhood

Simplify the behavior of ./pants fmt.isort <empty> because release 1.2.0 is already saying the following, so this change makes it happen.

WARN] The behavior of `./pants fmt.isort` (no explicit targets) will soon become a no-op. To remove this warning, please specify one or more explicit target specs (e.g. `./pants fmt.isort ::`).

Behavior:
./pants fmt.isort <targets> -- <args, e.g. "--recursive ."> will sort the files only related
to specified targets, but the way of finding the config(s) is vanilla. If no target is
specified or no python source file is found in <targets>, it would be a no-op.

  • Add check for sources with target specified, otherwise fmt.isort a Java target will result empty python sources with the following error:
$ ./pants fmt.isort examples/tests/java/org/pantsbuild/example/hello/greet/:: -- --check-only

23:41:24 00:00 [main]
               (To run a reporting server: ./pants server)
23:41:24 00:00   [setup]
23:41:24 00:00     [parse]
               Executing tasks in goals: bootstrap -> fmt
23:41:24 00:00   [bootstrap]
23:41:24 00:00     [substitute-aliased-targets]
23:41:24 00:00     [jar-dependency-management]
23:41:24 00:00     [bootstrap-jvm-tools]
23:41:24 00:00     [provide-tools-jar]
23:41:24 00:00   [fmt]
23:41:24 00:00     [isort]ERROR: /Users/yic/workspace/pants/build-support/isort.venv/bin/activate_this.py Imports are incorrectly sorted.
ERROR: /Users/yic/workspace/pants/build-support/isort.venv/lib/python2.7/_abcoll.py Imports are incorrectly sorted.
ERROR: /Users/yic/workspace/pants/build-support/isort.venv/lib/python2.7/codecs.py Imports are incorrectly sorted.
ERROR: /Users/yic/workspace/pants/build-support/isort.venv/lib/python2.7/linecache.py Imports are incorrectly sorted.
...

Other minor change:
* Add back one target name for intellij pants plugin missing deps test, as it currently depends on the name to function.

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

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  1. I suggest a slightly simpler check, for you to take or leave.

  2. The original code conflates args with sources. If you just change if len(args) == 0: to if not sources: I think it will catch the problem more precisely, and with less work.

    1. The behavior has changed a bit. Hopefully this check makes more sense now with args being gone.

  3. If you decide to take my rec above, how about retitling to something like test_no_python_sources_causes_noop.

    1. iiuc this should be test_isort_no_python_sources_should_noop, as cause may imply a problem so we should do the opposite of that.

  4. 
      
  1. Ship It!
    1. I changed the behavior quite a bit on the 3rd revision. May be worth another look.

  2. 
      
  1. I think I preferred the way this task worked in the first revision. From my point of view, all fmt and lint goals should respect target_roots, which it looks like this second revision removes.

    I think that the original suggestion of simply s/arg/sources/ on line 62:
    * fixes the original issue
    * makes ./pants fmt.isort noop
    * also allows to fmt only target_roots

    It is possible I am missing a subtlety - let me know if so.

    1. Yes it is a bug, and target_roots should be used. Will fix.

      It is also the case now ./pants fmt.isort will sort the entire directory, so will fix that too.

      To summarize:
      1. ./pants fmt.isort <empty target> -- <args> will be no-op because there are not sources.
      2. ./pants fmt.isort <some target> -- <args> will be no-op if there are no python sources.
      3. Essentially getting rid of the vanilla mode of isort. I guess that makes sense, because people can just call isort directly in the repo if that's the case instead of using pants.

    2. So yes I'd agree if sources: should suffice for the check.

    3. corrected.

  2. I think that 1. is now a lie - the changes appear to now operate over every Python target in context.

  3. actually, isn't this a lie now too? It is a little tough to grok the docs, imo.

    But I think your original repro demonstrated that the "vanilla" for isort is to descend the cwd when passed no files.

    But you now short circuit the invocation if no sources are found.

    1. this mode has been removed.

  4. FWIW, my point on the earlier review is that not sources is all that you need to test here. The state of targets becomes irrelevant.

  5. Why log to info? That is a pretty heavy weight move, surpressing that output becomes a tough choice to make.

  6. 
      
  1. Thanks, looks good!

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

45e4e6bb334e4b82b61c126f84c5d13e04dcbbf5 thanks gents!

Loading...