-
Notifications
You must be signed in to change notification settings - Fork 653
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
Fix remaining names in CONTRIBUTING.md #70
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,3 +41,4 @@ Makefile.dep | |
.ccls-cache/* | ||
compile_commands.json | ||
redis.code-workspace | ||
.cache |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,14 +1,14 @@ | ||||||||||||||||||||||||
Note: by contributing code to the Redis project in any form, including sending | ||||||||||||||||||||||||
Note: by contributing code to the Valkey project in any form, including sending | ||||||||||||||||||||||||
a pull request via GitHub, a code fragment or patch via private email or | ||||||||||||||||||||||||
public discussion groups, you agree to release your code under the terms | ||||||||||||||||||||||||
of the Redis license that you can find in the COPYING file included in the Redis | ||||||||||||||||||||||||
of the Valkey license that you can find in the COPYING file included in the Valkey | ||||||||||||||||||||||||
source distribution. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
# IMPORTANT: HOW TO USE REDIS GITHUB ISSUES | ||||||||||||||||||||||||
# IMPORTANT: HOW TO USE VALKEY GITHUB ISSUES | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
GitHub issues SHOULD ONLY BE USED to report bugs and for DETAILED feature | ||||||||||||||||||||||||
requests. Everything else should be asked on Discord: | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
https://discord.gg/zbcPa5umUB | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
PLEASE DO NOT POST GENERAL QUESTIONS that are not about bugs or suspected | ||||||||||||||||||||||||
|
@@ -27,9 +27,14 @@ If you are reporting a security bug or vulnerability, see SECURITY.md. | |||||||||||||||||||||||
|
||||||||||||||||||||||||
## Developer Certificate of Origin | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
We respect the intellectual property rights of others and we want to make sure all incoming contributions are correctly attributed and licensed. A Developer Certificate of Origin (DCO) is a lightweight mechanism to do that. The DCO is a declaration attached to every commit. In the commit message of the contribution, the developer simply adds a `Signed-off-by` statement and thereby agrees to the DCO, which you can find below or at [DeveloperCertificate.org](http://developercertificate.org/). | ||||||||||||||||||||||||
We respect the intellectual property rights of others and we want to make sure | ||||||||||||||||||||||||
all incoming contributions are correctly attributed and licensed. A Developer | ||||||||||||||||||||||||
Certificate of Origin (DCO) is a lightweight mechanism to do that. The DCO is | ||||||||||||||||||||||||
a declaration attached to every commit. In the commit message of the contribution, | ||||||||||||||||||||||||
the developer simply adds a `Signed-off-by` statement and thereby agrees to the DCO, | ||||||||||||||||||||||||
which you can find below or at [DeveloperCertificate.org](http://developercertificate.org/). | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
Comment on lines
+31
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving linebreaks to beween sentences and after comma in some cases. This linebreak has no effect on the rendered output on the web page. It's per the valkey-doc style guidelines.
Suggested change
|
||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
```text | ||||||||||||||||||||||||
Developer's Certificate of Origin 1.1 | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
By making a contribution to this project, I certify that: | ||||||||||||||||||||||||
|
@@ -56,14 +61,21 @@ By making a contribution to this project, I certify that: | |||||||||||||||||||||||
sign-off) is maintained indefinitely and may be redistributed | ||||||||||||||||||||||||
consistent with this project or the open source license(s) | ||||||||||||||||||||||||
involved. | ||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
We require that every contribution to Valkey to be signed with a DCO. We require the usage of known identity (such as a real or preferred name). We do not accept anonymous contributors nor those utilizing pseudonyms. A DCO signed commit will contain a line like: | ||||||||||||||||||||||||
We require that every contribution to Valkey to be signed with a DCO. We require the | ||||||||||||||||||||||||
usage of known identity (such as a real or preferred name). We do not accept anonymous | ||||||||||||||||||||||||
contributors nor those utilizing pseudonyms. A DCO signed commit will contain a line like: | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
Comment on lines
+66
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the styling guide in valkey-doc:
Suggested change
|
||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
```text | ||||||||||||||||||||||||
Signed-off-by: Jane Smith <[email protected]> | ||||||||||||||||||||||||
``` | ||||||||||||||||||||||||
You may type this line on your own when writing your commit messages. However, if your user.name and user.email are set in your git configs, you can use `git commit` with `-s` or `--signoff` to add the `Signed-off-by` line to the end of the commit message. We also require revert commits to include a DCO. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
You may type this line on your own when writing your commit messages. However, if your | ||||||||||||||||||||||||
user.name and user.email are set in your git configs, you can use `git commit` with `-s` | ||||||||||||||||||||||||
or `--signoff` to add the `Signed-off-by` line to the end of the commit message. We also | ||||||||||||||||||||||||
require revert commits to include a DCO. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
Comment on lines
+75
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
# How to provide a patch for a new feature | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -74,7 +86,8 @@ and creating an issue at Github with the description of, exactly, what you want | |||||||||||||||||||||||
to accomplish and why. Use cases are important for features to be accepted. | ||||||||||||||||||||||||
Here you can see if there is consensus about your idea. | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
2. If in step 1 you get an acknowledgment from the project leaders, use the following procedure to submit a patch: | ||||||||||||||||||||||||
2. If in step 1 you get an acknowledgment from the project leaders, use the following | ||||||||||||||||||||||||
procedure to submit a patch: | ||||||||||||||||||||||||
1. Fork Valkey on GitHub ( https://docs.github.com/en/github/getting-started-with-github/fork-a-repo ) | ||||||||||||||||||||||||
Comment on lines
+89
to
+90
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
1. Create a topic branch (git checkout -b my_branch) | ||||||||||||||||||||||||
1. Make the needed changes and commit with a DCO. (git commit -s) | ||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got kind of stuck on this when I was originally writing it. Maybe we split it by sentence instead of by some weird character count? That is how redis-doc used to work, I don't really like these artificial line breaks (since they aren't necessary) and sometimes makes people try to fix the width when they change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line was very long. It is hard to read even on a large screen. Also the line breaks don't have effect on the final rendering.
Are you suggesting something like the below? using an empty line to emulate the line break. Do you suggest breaking on the complete sentence only or it is ok to break after punctuations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as a line break after each sentence. Also encourages shorter sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we should aim for short sentences but it feels unnecessary coupling to me. I also don't think this is how the old codebase/doc is done (and we know consistency wasn't a strength anyways). I feel most people would be more accustomed to a fixed (or relatively fixed) line width. Maybe now it is a good time to get alignment? What do you think about 120 characters per line for everything, doc and code? FWIW, I would be happy with anything between 100-140.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree generally at 120 character lines, but I'm not sure it applies to markdown files. Valkey-doc is the counter point that was pretty consistent, and it is all markdown (see https://github.com/valkey-io/valkey-doc?tab=readme-ov-file#styling-guidelines). The problem with 120 character lines is we don't enforce it though, we should add a linter like was suggested.
I'm fine to pick a new style, but think we should stick to it moving forward. (Since, I don't really like losing the history on this specific thing, if people wonder when we added the DCO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In markdown files, a blank line makes a new paragraph. A single line break has no effect on the rendered HTML. It doesn't create a line break. It's just for source code readability. So when we don't want a new paragraph, just do a linebreak between sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG.
I think there is a way to enforce the new style on the new code. I will experiment with clang-format next and see if I can set it up in a way that is automatically applied to the code changes only during the merge. Will tackle it in separate PRs.
I think this is the beauty of markdown and you can write however you like but still render perfectly. The problem I see is for someone who needs to work on the raw content, it becomes challenging to deal with super long lines. In this specific case, the styling changes were done on the DCO section, which is added right before this PR so I think the history part is ok. Also I feel that the doc history is less critical as the code history, in general.
Lastly, I think the history preserving problem should be solved by the proper tooling. The diffing tool needs to be smart enough to understand the language it deals with and ideally has an option to show semantics differences only. I will do some research in the background next.