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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add clang-format script and precommit hook #12651

Merged
merged 16 commits into from Apr 27, 2018

Conversation

Projects
None yet
4 participants
@codebytere
Member

codebytere commented Apr 18, 2018

Adds new scripts lint-clang-format and clang-format that check for incorrectly formatted code and format code for clang-format standards, respectively.

Also adds clang-format as a dev dependency.

Should NOT be merged until the other formatting PRs are merged 馃檯

/cc @ckerr @alexeykuzmin @nornagon @MarshallOfSound

@codebytere codebytere requested a review from electron/reviewers as a code owner Apr 18, 2018

@zeke

zeke approved these changes Apr 18, 2018

@@ -68,6 +71,11 @@
"start": "python ./script/start.py",
"test": "python ./script/test.py"
},
"husky": {
"hooks": {
"precommit": "npm run check-clang-format \"npm run clang-format\""

This comment has been minimized.

@zeke

zeke Apr 18, 2018

Member

husky also supportsscripts.precommit as an alternative to husky.hooks.precommit. But what you have here is more explicit and less magic. I don't feel strongly, but If we stick to this format, then the existing prepack and prepush scripts should probably be moved here too.

@zeke

This comment has been minimized.

Member

zeke commented Apr 18, 2018

This is awesome! Resolves #12391

@alexeykuzmin

alexeykuzmin requested changes Apr 18, 2018 edited

  1. I get no feedback from the pre-commit script, only the fact that the changes didn't pass the check:
$ git commit -m 'test'
husky > npm run -s precommit (node v6.11.3)


husky > pre-commit hook failed (add --no-verify to bypass)
  1. It seems like the script checks all files, but should check only new and modified lines that are going to be commited.
    It won't be possible to make changes in the docs for instance if somehow there are incorrectly formatted C++ code somewhere.

Can git-clang-format help here?

@ckerr

This comment has been minimized.

Member

ckerr commented Apr 18, 2018

The existing cpplint.py script has a -c option to only lint changed files. Maybe that code can be reused here to address @alexeykuzmin's second point about running it on changed files rather than all files?

@codebytere

This comment has been minimized.

Member

codebytere commented Apr 18, 2018

Working on that now!

codebytere added some commits Apr 18, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Apr 19, 2018

Should be finished now; only checks for proper clang-formatting in the precommit hook and outputs a descriptive error in the event of a failure.

@@ -0,0 +1,316 @@
#!/usr/bin/env python

This comment has been minimized.

@zeke

zeke Apr 19, 2018

Member

@codebytere out of curiosity, did you write this whole wrapper from scratch, or is it derived from some existing thing? I ask because I'm wondering if we could just install some "egg" and be done with it, though I don't know if there's a precedent for that in this project. I think all the python code is just one-off scripts.

This comment has been minimized.

@codebytere

codebytere Apr 20, 2018

Member

i derived it from https://github.com/Sarcasm/run-clang-format, but the intended usage for that is to be dropped into a project like i did here

i ended up submitting PRs for the changes i made to it actually haha

@ckerr

ckerr approved these changes Apr 20, 2018

script/run-clang-format.py is a pretty slick wrapper -- pass in arbitrary files, do changed files only, or format the whole world. 馃憤 on this, once it goes green

codebytere added some commits Apr 20, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Apr 20, 2018

Before this is merged i'd like to quickly chat about .mm files, as we'll need to change this accordingly.

codebytere added some commits Apr 20, 2018

codebytere added some commits Apr 20, 2018

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 24, 2018

At least one more blocker. The hooks tries to check removed files and fails with run-clang-format.py: error: [Errno 2] No such file or directory: '<...>' error.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 24, 2018

Btw, does the whole thing work on Windows?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 24, 2018

And I still don't like the fact that it checks the whole file, not only the staged chunks (the ones that are actually going to be committed) =/

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Apr 27, 2018

And I still don't like the fact that it checks the whole file, not only the staged chunks (the ones that are actually going to be committed) =/

I guess we can work own it later.

@codebytere codebytere merged commit 21e5a2e into master Apr 27, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@codebytere codebytere deleted the add-clang-format-hook branch Apr 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment