Skip to content

Prevent clang-format in certain places#468

Merged
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
zuiderkwast:clang-format-off
May 8, 2024
Merged

Prevent clang-format in certain places#468
zuiderkwast merged 2 commits into
valkey-io:unstablefrom
zuiderkwast:clang-format-off

Conversation

@zuiderkwast

@zuiderkwast zuiderkwast commented May 8, 2024

Copy link
Copy Markdown
Contributor

This is a preparation for adding clang-format.

These comments prevent automatic formatting in some places. With these exceptions, we will be able to run clang-format on the rest of the code.

This is a preparation for #323.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast zuiderkwast requested review from PingXie and madolson May 8, 2024 14:15
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@codecov

codecov Bot commented May 8, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 68.91%. Comparing base (315b757) to head (3c76e9b).

Additional details and impacted files
@@            Coverage Diff            @@
##           unstable     #468   +/-   ##
=========================================
  Coverage     68.90%   68.91%           
=========================================
  Files           109      109           
  Lines         61793    61792    -1     
=========================================
+ Hits          42579    42584    +5     
+ Misses        19214    19208    -6     
Files Coverage Δ
src/aof.c 80.79% <ø> (ø)
src/asciilogo.h 100.00% <ø> (ø)
src/eval.c 55.45% <ø> (ø)
src/functions.c 95.62% <ø> (ø)
src/latency.c 81.49% <ø> (ø)
src/listpack.c 90.19% <ø> (ø)
src/module.c 9.34% <ø> (ø)
src/networking.c 85.08% <ø> (ø)
src/notify.c 97.01% <ø> (ø)
src/object.c 78.41% <100.00%> (ø)
... and 13 more

... and 8 files with indirect coverage changes

@hwware

hwware commented May 8, 2024

Copy link
Copy Markdown
Contributor

Before merging this PR, can we just clarify under which condition, developer should /* clang-format off */ flag?
I think we should describe this in some places so that all people can follow this rule.

@zuiderkwast

Copy link
Copy Markdown
Contributor Author

Before merging this PR, can we just clarify under which condition, developer should /* clang-format off */ flag? I think we should describe this in some places so that all people can follow this rule.

We can describe all coding rules. Now we don't have any written rules. But I don't think it should be a blocker. This (clang-format off) will probably be very rare for new code. We can do it for each PR, if it ever comes up.

@PingXie

PingXie commented May 8, 2024

Copy link
Copy Markdown
Member

The whole point of applying clang-format is to get consistency in our code base without every committer having to read through a long documentation and reviewers spending their brain cycles in nitpicking styling issues :-).

@zuiderkwast

Copy link
Copy Markdown
Contributor Author

Right, so we can say something in CONTRIBUTING.md that we use clang-format. Disabling it should normally not be done but we can accept it in cases similar to the ones in this PR. Is that OK or do you have another idea?

@zuiderkwast

Copy link
Copy Markdown
Contributor Author

Btw, coding style can also include naming, camel/snake case and more, not covered by clang-format.

@hwware hwware added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label May 8, 2024
@hwware

hwware commented May 8, 2024

Copy link
Copy Markdown
Contributor

I would like to add one tag for this pr for documentation later. No other opinion so far.
We could merge it.

@zuiderkwast zuiderkwast merged commit 6af51f5 into valkey-io:unstable May 8, 2024
@zuiderkwast zuiderkwast deleted the clang-format-off branch May 8, 2024 18:59
@madolson

madolson commented May 8, 2024

Copy link
Copy Markdown
Member

I would like to add one tag for this pr for documentation later. No other opinion so far.

What documentation do we need to add?

@madolson

madolson commented May 8, 2024

Copy link
Copy Markdown
Member

(The PR looked fine to me)

@zuiderkwast

Copy link
Copy Markdown
Contributor Author

I would like to add one tag for this pr for documentation later. No other opinion so far.

What documentation do we need to add?

We can describe when it's OK vs not OK to use /* clang-format off */. This probably belongs in CONTRIBUTING.md or similar. Not in the doc repo.

(The PR looked fine to me)

Sorry I was eager to merge it. :)

karthyuom pushed a commit to karthyuom/valkey that referenced this pull request May 13, 2024
This is a preparation for adding clang-format.

These comments prevent automatic formatting in some places. With these
exceptions, we will be able to run clang-format on the rest of the code.

This is a preparation for valkey-io#323.

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Signed-off-by: Karthick Ariyaratnam <karthyuom@gmail.com>
srgsanky pushed a commit to srgsanky/valkey that referenced this pull request May 19, 2024
This is a preparation for adding clang-format.

These comments prevent automatic formatting in some places. With these
exceptions, we will be able to run clang-format on the rest of the code.

This is a preparation for valkey-io#323.

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants