Skip to content

Conversation

ki4070ma
Copy link
Collaborator

It's to keep imports clean.

ci.sh Outdated
fi

result=$(python -m isort -rc . | grep -v "Skipped")
if [[ $result ]] ; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do we check here?

Copy link
Collaborator Author

@ki4070ma ki4070ma May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking if unordered imports aren't remained.
python -m isort -rc . also outputs diff when unordered imports remained.
To cover the case that developers don't set pre-commit and uploaded files have unordered imports. (Same to autopep8's check)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we check the exit code instead ($?) ?

Copy link
Collaborator Author

@ki4070ma ki4070ma May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment is exact.
I've checked its behavior at first, but unfortunately both cases(ordered, unordered) return 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@ki4070ma ki4070ma May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will look into again.
At least I found below issue(fixed) and codes. Sholud be returned non 0.

PyCQA/isort#423
https://github.com/timothycrosley/isort/blob/d093f97c1d26854fbb082e43cfae8a062dacda56/isort/main.py#L382

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do as expected by using --check-only opt for isort :)

@ki4070ma ki4070ma merged commit e43e54d into appium:master May 17, 2019
@ki4070ma ki4070ma deleted the isort branch May 17, 2019 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants