CI: Format isort output for azure #29654
Conversation
alimcmaster1
added
CI
There was a problem hiding this comment.
lgtm
Thanks for picking this up! FYI for smaller changes we typically try to push to the original contributors branch so they get credit (I use hub to do this). By no means required just something to consider
@datapythonista @jbrockmendel if you care to look
There was a problem hiding this comment.
Good stuff. Some minor stylistic comments, but looks good.
I think the error in the CI is missing some space that you may want to add in the awk expression too.
@WillAyd, I don't think @alimcmaster1 has got the permissions to push to other people PRs. @alimcmaster1 you can still give credits by adding co-authors to you commits I think. I don't remember the syntax, and it's not important. Just in case you think it's worth the research (probably not).
ci/code_checks.sh
Outdated
| @@ -109,7 +109,13 @@ if [[ -z "$CHECK" || "$CHECK" == "lint" ]]; then | |||
|
|
|||
| # Imports - Check formatting using isort see setup.cfg for settings | |||
| MSG='Check import format using isort ' ; echo $MSG | |||
| isort --recursive --check-only pandas asv_bench | |||
| set -o pipefail | |||
There was a problem hiding this comment.
I think you should disable when it's not needed anymore. Otherwise things in other part of the script can fail, and this is not expected to be active by default.
ci/code_checks.sh
Outdated
| @@ -109,7 +109,13 @@ if [[ -z "$CHECK" || "$CHECK" == "lint" ]]; then | |||
|
|
|||
| # Imports - Check formatting using isort see setup.cfg for settings | |||
| MSG='Check import format using isort ' ; echo $MSG | |||
| isort --recursive --check-only pandas asv_bench | |||
| set -o pipefail | |||
| isort_cmd="isort --recursive --check-only pandas asv_bench" | |||
There was a problem hiding this comment.
I think the convention is to have environment variables in bash in upper case.
ci/code_checks.sh
Outdated
| if [[ "$AZURE" == "true" ]]; then | ||
| eval $isort_cmd | awk '{ORS=""} {print "##vso[task.logissue type=error;sourcepath="$2";]"} {$1=$2=""; print $0}' | ||
| else | ||
| eval $isort_cmd |
There was a problem hiding this comment.
This indentation doesn't much the previous eval.
ci/code_checks.sh
Outdated
| set -o pipefail | ||
| isort_cmd="isort --recursive --check-only pandas asv_bench" | ||
| if [[ "$AZURE" == "true" ]]; then | ||
| eval $isort_cmd | awk '{ORS=""} {print "##vso[task.logissue type=error;sourcepath="$2";]"} {$1=$2=""; print $0}' |
There was a problem hiding this comment.
Just an opinion, but I'd personally prefer something like:
if [[ "$AZURE" == "true" ]]; then
FORMAT_ISORT="awk '{ORS=""} {print "##vso[task.logissue type=error;sourcepath="$2";]"} {$1=$2=""; print $0}'"
else
FORMAT_ISORT="cat"
fi
isort --recursive --check-only pandas asv_bench | eval $FORMAT_ISORTNot a huge difference, but I think it makes more clear what's going on, since the line that actually does things is just one, shorter, and more explicit on what it does
datapythonista
changed the title
|
Since we are now using Github Actions ill update this. Before this change output is: |
|
Output now looks better - but I need to fix the return code |
ci/code_checks.sh
Outdated
| isort --recursive --check-only pandas asv_bench | ||
| ISORT_CMD="isort --recursive --check-only pandas asv_bench" | ||
| if [[ "$GITHUB_ACTIONS" == "true" ]]; then | ||
| eval $ISORT_CMD | awk '{print "##[error]" $0}' |
There was a problem hiding this comment.
| eval $ISORT_CMD | awk '{print "##[error]" $0}' | |
| eval $ISORT_CMD | awk '{print "##[error]" $0}' ; $?=${PIPESTATUS[0]} |
Not sure if you can do something like this, or similar, but this will consider the return of awk to fail the CI, and will always be 0.
There was a problem hiding this comment.
Thanks for this v. handy
Co-Authored-By: Marc Garcia <[email protected]>
|
Related failures: #30031 |
|
CI finally passed. @datapythonista @WillAyd to merge |
|
Thanks @WillAyd |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffCurrent behaviour on master of isort formatting:
Finishing up stale PR @#27334