Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve logging of build-support/isort.sh helper script #7503

Merged
merged 4 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@Eric-Arellano
Copy link
Contributor

commented Apr 5, 2019

Problem

isort.sh suffers from two problems:

  1. When not using -f, the output upon failure is unnecessarily wordy and does not say which files were the actual issue. For example:
FAILURE: Exited with return code 1 while running `/Users/eric/.pyenv/shims/python3.6 /Users/eric/.cache/pants/python/CPython-3.6.8/d6bcba436bdefbc27a69dd880d6ee5716e004764/isort-c201e856817dc9ae802d18e3ec45a3bfa4c2ac11.pex --check-only src/python/pants/util/tarutil.py src/python/pants/util/retry.py src/python/pants/util/osutil.py src/python/pants/util/memo.py src/python/pants/util/process_handler.py src/python/pants/util/strutil.py src/python/pants/util/py2_compat.py src/python/pants/util/meta.py src/python/pants/util/xml_parser.py src/python/pants/util/objects.py src/python/pants/util/rwbuf.py src/python/pants/util/collections_abc_backport.py`.

FAILURE
  1. When using -f, we swallow all output. Presumably, the user would like to know which files were changed, rather than having to run git status to do this.

Solution

No longer run the command as --quiet and use grep to parse out lines we care about.

Result

If --fix is passed and files were fixed, we will output with return code 0:

Fixing /Users/eric/DocsLocal/code/projects/pants/src/python/pants/util/strutil.py

If --fix is left off, and files had bad sort order, we will output with return code 1:

ERROR: /Users/eric/DocsLocal/code/projects/pants/src/python/pants/util/strutil.py Imports are incorrectly sorted.
@jsirois

jsirois approved these changes Apr 5, 2019

Copy link
Member

left a comment

Yay simplicity!

Try to fix issue with return codes
If the grep search failed, we would return an error code of 1 unintentionally.
@cosmicexplorer
Copy link
Contributor

left a comment

Thank you for fixing this issue! I will be adding a test for it soon, once I get back to the other BUILD file header checks I want to do. This is a great change!

@Eric-Arellano Eric-Arellano merged commit b170379 into pantsbuild:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:isort-round-2 branch Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.