Document the clang-format flow in CONTRIBUTING.md#659
Document the clang-format flow in CONTRIBUTING.md#659SoulPancake wants to merge 3 commits intovalkey-io:unstablefrom
Conversation
Signed-off-by: Anuragkillswitch <[email protected]>
SoulPancake
force-pushed
the
ab/issue-649-document-clang-format-flow
branch
from
0cd3972 to
1a9dff4
Compare
|
@PingXie Can you please take a look ^_^ |
enjoy-binbin
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #659 +/- ##
============================================
+ Coverage 70.17% 70.19% +0.01%
============================================
Files 110 110
Lines 60077 60078 +1
============================================
+ Hits 42160 42171 +11
+ Misses 17917 17907 -10 |
| [Clang Format Check/clang-format-check] 🏁 Job succeeded | ||
| ``` | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We should probably keep the DCO at the top as this is the most important information IMO for any new contributor. Also while you are at it, can you add some git commands to show how to add the DCO? I noticed that the commands were not obvious to some folks in the past.
Co-authored-by: Binbin <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
| * Want to help with documentation? [Move on to valkey-doc](https://github.com/valkey-io/valkey-doc) | ||
| * Report a vulnerability? See [SECURITY.md](SECURITY.md) | ||
|
|
||
| ## How to Install clang-format 18 on a Debian Box |
There was a problem hiding this comment.
I don't really want to maintain an installation guide of clang-format for debian in this repository. Is there some better place we can point to? Do we need at least clang-format version 18 for this to run correctly?
There was a problem hiding this comment.
yes 18 is the most compatible one. I think we can go higher but the installation becomes challenging.
What would you recommend for this documentation? Would it work we clarify this being debian specific (so we can extend it in the future to other platforms)?
There was a problem hiding this comment.
I install clang-format version 14 on Ubuntu 22, it works fine. But I have no chance to older version, So we can say:
First run git clang-format, If you still meet an error, please upgrade your clang-format to latest version.
|
|
||
| ## How to Run clang-format-check Action Locally | ||
|
|
||
| 1. Get [`act`](https://github.com/nektos/act). |
There was a problem hiding this comment.
| 1. Get [`act`](https://github.com/nektos/act). | |
| 1. Install [`act`](https://nektosact.com/installation/index.html). |
| 2. Commit the changes locally: | ||
| ```sh | ||
| git commit -a -s | ||
| ``` |
There was a problem hiding this comment.
| 2. Commit the changes locally: | |
| ```sh | |
| git commit -a -s | |
| ``` |
Instead, I think you should indicate it runs on what is current committed locally.
| * Want to help with documentation? [Move on to valkey-doc](https://github.com/valkey-io/valkey-doc) | ||
| * Report a vulnerability? See [SECURITY.md](SECURITY.md) | ||
|
|
||
| ## How to Install clang-format 18 on a Debian Box |
There was a problem hiding this comment.
It would also be good to put this under a code format section, instead of a section for installing clang format. It would be nice to include an example for how to run it.
| sudo act -j clang-format-check | ||
| ``` | ||
|
|
||
| ### Here is a Failure Output |
There was a problem hiding this comment.
I think these two sections are overkill. The most useful part is the git diff part, so maybe just keep that? If there is no diff, then it will pass.
|
I was wondering if we can do something like the following for the installation steps @PingXie @madolson @enjoy-binbin @hwware |
|
The 30 second demo thing look cool, but i don't like this kind of fancy stuff on projects |
The 30-second demo GIF makes sense for a dev tool project to show users how it makes the workflow smooth. However, including it as part of a contributing document may not be particularly useful. 🤔 |
6 participants
Solves #649.