Skip to content

Source Code Format Checks #380

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

Merged
merged 9 commits into from
Apr 23, 2021
Merged

Source Code Format Checks #380

merged 9 commits into from
Apr 23, 2021

Conversation

DellaBitta
Copy link
Contributor

@DellaBitta DellaBitta commented Apr 16, 2021

This change includes some tools to keep a consistent format of the repository's source files, including:

  • a Checks workflow which errors if any of a branch's changed files require formatting. We can extend this workflow with copyright header checks in the future and the pre-existing Check PR Labels has been folded into it.
  • updates to install_prereqs_desktop.py to ensure that clang-format is installed.
  • updates to scripts/format_code.py to reduce logging in non-verbose mode.
  • a pre-commit githook. Note that the githook has to be installed manually by copying it from scripts/git/hooks/pre-commit to .git/hooks/pre-commit
  • an update to our Contribution guidelines with formatting instructions.

@google-cla google-cla bot added the cla: yes label Apr 16, 2021
@jonsimantov
Copy link
Contributor

Suggestion (possibly for a new PR) - add a label to autoformat a PR and push the changes in another commit, in case the developer doesn't want to run the formatter locally.

@@ -0,0 +1,30 @@
name: Consistency Checks
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should merge Check PR Labels into this, to reduce the number of workflows running. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can do that tomorrow morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@DellaBitta
Copy link
Contributor Author

DellaBitta commented Apr 20, 2021

Suggestion (possibly for a new PR) - add a label to autoformat a PR and push the changes in another commit, in case the developer doesn't want to run the formatter locally.

Just a heads up that the PR that we've been reviewing from a third party user submission has had issues with our itest labels.
The issues stem from the fact that all of the workflows are being run from the user's github account, not ours, and they don't have write permissions on firebase repos. This prevents some of the label steps from completing successfully -- - maybe only the ones which post comments..?

I don't know if that applies at all to what you're suggesting, but I think that we need to retool the label control & flow that we already have.

Copy link
Contributor

@vimanyu vimanyu left a comment

Choose a reason for hiding this comment

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

minor python readability comments.

@jonsimantov
Copy link
Contributor

Suggestion (possibly for a new PR) - add a label to autoformat a PR and push the changes in another commit, in case the developer doesn't want to run the formatter locally.
Just a heads up that the PR that we've been reviewing from a third party user submission has had issues with our itest labels.

Yeah, I'm aware of those issues. I still feel that this is worth doing for our internal development. Also, I have been thinking about switching over to using firebase-workflow-trigger's token to make the changes, where we were previously using the github-action default token to do. Do you know if the itest runs from forked repos have access to our repo's secrets?

Copy link
Contributor

@vimanyu vimanyu left a comment

Choose a reason for hiding this comment

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

LGTM!

@DellaBitta DellaBitta merged commit 8f3e192 into main Apr 23, 2021
@DellaBitta DellaBitta deleted the feature/ddb-format-checks branch April 23, 2021 17:37
@firebase firebase locked and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants