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

Fix remaining names in CONTRIBUTING.md #70

Merged
merged 1 commit into from
Mar 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,4 @@ Makefile.dep
.ccls-cache/*
compile_commands.json
redis.code-workspace
.cache
35 changes: 24 additions & 11 deletions CONTRIBUTING.md
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
Expand All @@ -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
Copy link
Member

@madolson madolson Mar 29, 2024

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.

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 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?

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/)

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@madolson madolson Mar 29, 2024

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.

Copy link
Contributor

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.

Copy link
Member Author

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

SG.

I don't really like losing the history on this specific thing

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.

Valkey-doc is the counter point that was pretty consistent

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.

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
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/).
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/).

```
```text
Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the styling guide in valkey-doc:

Suggested change
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:

```

```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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

# How to provide a patch for a new feature

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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. Create a topic branch (git checkout -b my_branch)
1. Make the needed changes and commit with a DCO. (git commit -s)
Expand Down
Loading