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

Use clang-format instead of cpplint #979

Merged
merged 17 commits into from
Oct 13, 2022

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented Oct 12, 2022

It closes the clang-format part of #800.

Note: Although it is not recommanded, but developers can disable clang-format for specific code snippets via:

// clang-format off

code that do not want to be formatted

// clang-format on

To format code:

./x.py format

To check whether code is formatted:

./x.py check format

To use a custom clang-format:

./x.py format --clang-format-path <path>

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

This changes seems in a good direction. Comments inline.

x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
.github/workflows/kvrocks.yaml Outdated Show resolved Hide resolved
@torwig
Copy link
Contributor

torwig commented Oct 12, 2022

@PragmaTwice Good job!

@git-hulk
Copy link
Member

@PragmaTwice Good job!

I also like this change, cpplint can't auto fix is too frustrate.

@git-hulk
Copy link
Member

@PragmaTwice Can you help to rebase those commits, I don't know which commit should I start looking.

@@ -0,0 +1,200 @@
---
Language: Cpp
# BasedOnStyle: Google
Copy link
Member

Choose a reason for hiding this comment

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

Generated? Could you share how you gain this config file?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. cd to a non-project directory
  2. clang-format-12 -style=google -dump-config > .clang-format
  3. change ColumnLimit to 120

Copy link
Member

@tisonkun tisonkun Oct 12, 2022

Choose a reason for hiding this comment

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

I'd prefer ColumnLimit to 200...Nowadays display device is far wider than 80 or 120 already, and wrap function calls or string literals are really clumsy.

Copy link
Member Author

Choose a reason for hiding this comment

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

The old limit is 120, so I just inherit it. I think maybe 200 is not friendly to my small 1080p laptop 🤣 , but I am open for it. What do other guys think about it? cc @git-hulk @torwig @caipengbo @ShooterIT

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think 120 is enough, we should rarely have too long lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think 120 is a good decision. Of course, we can use even 200, but 120 makes developers think if the length of the line is more than 120 characters.

Copy link
Member

Choose a reason for hiding this comment

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

I also think 120 is enough.

@PragmaTwice
Copy link
Member Author

@PragmaTwice Can you help to rebase those commits, I don't know which commit should I start looking.

You can just review three file: x.py, .github/workflows/kvrocks.yml and .clang-format, all of other file changes are made by clang-format.

@torwig
Copy link
Contributor

torwig commented Oct 12, 2022

@PragmaTwice You can squash a bunch of fix commits so they won't be merged in the unstable branch in the current representation :)

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thanks for your updates! Two comments inline.

x.py Outdated Show resolved Hide resolved
x.py Outdated Show resolved Hide resolved
@tisonkun
Copy link
Member

@PragmaTwice You can squash a bunch of fix commits so they won't be merged in the unstable branch in the current representation :)

@torwig I think we're using squash and merge here :)

x.py Outdated Show resolved Hide resolved
@torwig
Copy link
Contributor

torwig commented Oct 12, 2022

@tisonkun Perhaps I'm too old, but I do "squashing" by myself :)

tisonkun
tisonkun previously approved these changes Oct 12, 2022
@tisonkun tisonkun merged commit 81d3792 into apache:unstable Oct 13, 2022
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

Successfully merging this pull request may close these issues.

6 participants