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

[Internal] clang-format the whole repo #25955

Closed
gengjiawen opened this issue Feb 6, 2019 · 4 comments
Closed

[Internal] clang-format the whole repo #25955

gengjiawen opened this issue Feb 6, 2019 · 4 comments

Comments

@gengjiawen
Copy link
Member

gengjiawen commented Feb 6, 2019

This is a sub task of #25908.

The whole repo means the src and test folder.

What I want is that let's take more time focus on code itself instead of chores like formatting.

Thought

  • Do we need to change current .clang-format rules.
    You can check current format change : gengjiawen@425befd

  • Many prs may need rebase. So merge large cpp related prs first ?

cc @joyeecheung @addaleax @refack @bnoordhuis @jasnell @cjihrig

@richardlau
Copy link
Member

You can check current format change : gengjiawen@425befd

Showing 196 changed files with 14,443 additions and 15,533 deletions.

IMO that's way too much churn and will cause issues with backports and blaming.

@gengjiawen
Copy link
Member Author

gengjiawen commented Feb 6, 2019

IMO that's way too much churn and will cause issues with backports and blaming.

Actually prettier or eslint whole repo is common thing, we have git history for blaming part.
You can see other repo do the similar thing.

copyed from #16115 (comment)

I want to make cpp format easier in the long term.
Now if I have cpp format issue after I write the code. After clang-format the whole file, I have to revert the changes not related the change, that's quite a lot of work. I handle this a couple of times, not enjoy doing this at all tbh.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 6, 2019

I agree with @richardlau , that was the reason I dropped #16122 in favor of #21997 which used the git-clang-format script to only format diffs instead of the whole repo. Unlike React or Babel we have a LTS support scheme and backport as much as possible, which makes this kind of formatting less feasible. (The git-clang-format approach was borrowed from electron under the advice from @codebytere)

Now if I have cpp format issue after I write the code. After clang-format the whole file, I have to revert the changes not related the change, that's quite a lot of work. I handle this a couple of times, not enjoy doing this at all tbh.

You can use git-clang-format for that, if you are on windows you can invoke the command manually (or add a shortcut similar to make format-cpp to vcbuild.bat). It could've been smarter if we had a convention to learn where the branch starts (like chromium's git cl format) but we do not have a complete toolchain to manage the life cycle of a PR branch yet.

@gengjiawen
Copy link
Member Author

@joyeecheung Thanks for the info.

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

No branches or pull requests

3 participants