Add black and isort for auto style formatting #58
Conversation
There was a problem hiding this comment.
This looks great! What I can't decide is whether it should apply the formatting on PR creation (and new commits to a PR), or just when the PR is merged to main. My instinct is the latter, so we don't have to be vigilant about the need to pull changes on a dev branch (to get the formatting changes) after every push to the branch. But if you feel strongly otherwise, I'll defer to you.
I don't have a strong preference for either. However, if we were to integrate some kind of testing to run on every pull request, I would recommend having the formatters run on open active PRs also so then the tests will run on the actual code that will be pushed to main. But for now, I think it would be simpler to have it run only on main. |
|
Great point on the testing. How about for now, we have it only apply to main, and if/when we implement testing, we switch to have it apply to PRs as well. Could you add a comment to the action .yml noting how we would make that switch? |
| steps: | ||
| - name: Determine Branch Name | ||
| run: | | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| echo "BRANCH_NAME=${{ github.head_ref }}" >> $GITHUB_ENV | ||
| else | ||
| BRANCH_NAME=$(echo ${GITHUB_REF#refs/heads/}) | ||
| echo "BRANCH_NAME=$BRANCH_NAME" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
The code for determining the branch name was only necessary if the workflow were to run on active PRs. This will not break the code but is no longer necessary. However, if we were to switch to have the workflow run on PRs then integrating that change will be very easy if we keep this code (addressed by the comment). Otherwise, it will take some work to change that behavior. Let me know if you think we should keep this code or not.
| steps: | ||
| - name: Determine Branch Name | ||
| run: | | ||
| if [ "${{ github.event_name }}" = "pull_request" ]; then | ||
| echo "BRANCH_NAME=${{ github.head_ref }}" >> $GITHUB_ENV | ||
| else | ||
| BRANCH_NAME=$(echo ${GITHUB_REF#refs/heads/}) | ||
| echo "BRANCH_NAME=$BRANCH_NAME" >> $GITHUB_ENV | ||
| fi |
There was a problem hiding this comment.
Same here as previous comment.
There was a problem hiding this comment.
Thanks, sounds good to keep the 'determine branch name' step in. I had one suggested change for the comment you added, and also would like to request that you add a comment at the top of the 'determine branch name' step explaining that it has no effect currently given this workflow only applies to the main branch, but will be important if/when we additionally apply this workflow to pull requests.
.github/workflows/isort.yml
Outdated
| @@ -1,10 +1,10 @@ | |||
| name: Code Formatter | |||
|
|
|||
| # Add `pull_request:` to apply on active PRs | |||
There was a problem hiding this comment.
Could you please change this to:
# Add 'pull_request:' beneath 'on:' to apply on active PRs
Isort is used to sort Python imports and black is used to format Python code. These two formatters will run on the code on every push to main and on any branch associated with an active pull request. It will then automatically push these changes to the current branch. If you'd like this behavior to change, please let me know and I can do that.
The automation since the isort workflow will be triggered on a push to main or an active pull request and after it finishes it will call the black workflow and black will be able to run. The two formatters run sequentially and not in parallel because one of the workflows may change the code and push that code to the current branch. And if the other workflow is also making changes there will be merge conflicts.
Also, since the workflows run on an active pull request, every time you push to an active pull request it would be nice to pull before working on it again since the formatters may make auto styling changes. Otherwise, merge conflicts can happen.