Skip to content
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

Switch to using `git-clang-format` for `make format` #165

Merged
merged 2 commits into from Oct 22, 2018

Conversation

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Oct 17, 2018

What does this PR do?

This PR updates the make format target use git-clang-format, which has been set to format only staged and unstaged changes, instead of formatting all the code in the repo.

This should avoid issues like in #164 where new users end up formatting lots of old code, and also stop the blame view getting messed up. However it does mean that make format needs to be run each commit (which it should be anyway).

There is a make format-all command that has the old behaviour, I expect we'll need to run this occasional to pick up any missing changes.

I'm not 100% certain this is better than how we do it currently, but I think it is. Thoughts?

How should this be manually tested?

Run make format and make format-all

I have,

  • updated CHANGELOG.md
  • updated the documentation
@tehn
tehn approved these changes Oct 17, 2018
@scanner-darkly
Copy link
Member

@scanner-darkly scanner-darkly commented Oct 17, 2018

the issue we had with #164 shouldn't really happen - as part of 3.0 PR i ran clang on all files (b2c8d16 and dc71c46 commits), precisely because there were several files that accumulated unformatted code over time. i have a feeling i didn't do it properly though, because i had to do it manually (since i'm on windows). likely it didn't use the proper .clang-format, which is weird because i manually copied it into each folder i ran from. i'll double check when i have a chance.

there is no way to prevent affecting blame though.. if code needs to be formatted we can't do it in a way that doesn't affect the history. i think it's more important to stay on top of it. but i think it's good to have specific make targets that will only format what was changed, and then we can do periodical formatting for anything that lapses.

we should also update PULL_REQUEST_TEMPLATE.md to add clang to the checklist.

@samdoshi samdoshi force-pushed the samdoshi:clang-format branch from 8fc9279 to 541bd45 Oct 18, 2018
@samdoshi samdoshi force-pushed the samdoshi:clang-format branch from 541bd45 to a72ca57 Oct 18, 2018
@samdoshi
Copy link
Collaborator Author

@samdoshi samdoshi commented Oct 18, 2018

Added a note about make format to PULL_REQUEST_TEMPLATE.md (screwed up the change a few times so had to force push...)

I'll merge in a few days assuming no objections

@samdoshi
Copy link
Collaborator Author

@samdoshi samdoshi commented Oct 18, 2018

FYI I'm pretty sure I can modify the .travis.yml file to fail if there are any differences between the repo and make format-all.

Any interest?

@scanner-darkly
Copy link
Member

@scanner-darkly scanner-darkly commented Oct 18, 2018

sounds like a good idea!

@samdoshi samdoshi merged commit 2b978ba into monome:master Oct 22, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samdoshi
Copy link
Collaborator Author

@samdoshi samdoshi commented Oct 22, 2018

I think this is now rebased and merged.

But GitHub is having a bad day. So the PR didn't close.

Will have another look in 24hrs and either merge again, or just close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants