-
Notifications
You must be signed in to change notification settings - Fork 100
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
ci: check code formatting on pull requests #953
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lionel-nj See #615 - I believe that this will fail for PRs using branches from forks due to lack of permissions to write to that branch from the fork. IIRC we tried a variation of this config and had that problem. See the history for this file too - https://github.com/MobilityData/gtfs-validator/commits/fb4939e29c584cccb4b46286e9f8fd27194ceed2/.github/workflows/formatting.yml.
If we want this to run on PRs versus the master, I think we'll just to implement it as a check that fails and then prompts the PR submitter to do the format themselves.
👍🏾 @barbeau I think that ab7add7 and 9bdf080 fix this. The workflow logs prompts the list of files that are not properly formatted - it no longer tries to commit on a branch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lionel-nj Thanks! This works, but I don't think it's very clear to the PR submitter what to do after it fails. First, they have to dig through the logs to find the error that caused the build to fail, and after than there are no instructions for what to do next to fix it.
Things that I think could improve this:
- Make the output more visible - Bots like the CLAbot comment on the PR to make it clear when the CLA has been signed and hasn't been signed. Ideally a bot would do the same with this.
- Add clear instructions for what to do next - If a bot in 1) above exists, it could comment with the exact command to run to format the files. If it does not, then adding output to the log with the exact command to run to format the files would be helpful. And maybe a link to the contribution documentation, if this is documented more there.
This contribution does not follow the conventions set by the Google Java style guide. Please download the formatter and run it with |
This contribution does not follow the conventions set by the Google Java style guide. Please download the formatter and run it with |
Could you include the full |
This contribution does not follow the conventions set by the Google Java style guide. Please download the formatter and run it with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @lionel-nj! Some thoughts in-line below.
- remove trailing whitespace Co-authored-by: Sean Barbeau <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @lionel-nj! 👍
Thanks for reviewing! |
relates to #615
Summary:
This PR modifies the
formatting
workflow so that it is executed in pull request rather than on push onmaster
branch.The goal here is to avoid cluttering the commit history of the master branch and make sure that code is formatted before it is merged.
Expected behavior:
Please check the workflow logs to have details about the list of misformatted files (e.g. https://github.com/MobilityData/gtfs-validator/runs/3313714565?check_suite_focus=true)
Please make sure these boxes are checked before submitting your pull request - thanks!
[ ] Run the unit tests withgradle test
to make sure you didn't break anything