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

Initial draft of contributing guide #1787

Open
wants to merge 2 commits into
base: unstable
Choose a base branch
from
Open
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
5 changes: 4 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -99,7 +99,10 @@ certain issues/PRs over others. If you think your issue/PR is very important
try to popularize it, have other users commenting and sharing their point of
view, and so forth. This helps.

4. For minor fixes, open a pull request on GitHub.
4. While developing code, make sure to refer to our [DEVELOPMENT_GUIDE.md](DEVELOPMENT_GUIDE.md),
which includes documentation about various best practices for writing Valkey code.

5. For minor fixes, open a pull request on GitHub.

To link a pull request to an existing issue, please write "Fixes #xyz" somewhere
in the pull request description, where xyz is the issue number.
50 changes: 50 additions & 0 deletions DEVELOPMENT_GUIDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Valkey development guidelines
This document provides an general overview for writing and designing code for Valkey.
During our long development history, we've made a lot of inconsistent decisions, but we strive to get incrementally better.

## General style guidelines
Most of the style guidelines are enforced by clang format, but some additional comments are included here.

1. C style comments `/* comment */` can be used for both single and multi-line comments. C++ comments `//` can only be used for single line comments.
1. Generally keep line lengths below 90 characters, however there is no explicit line length enforcement.
Copy link
Member

Choose a reason for hiding this comment

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

I believe we landed on 120'ish previously but I am personally more in favor of 90 .

1. Use static functions when a function is only intended to be accessed from the same file.
Copy link
Member

Choose a reason for hiding this comment

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

should we also outlaw the "_" prefix explicitly now? We can fix the existing ones gradually.

@zuiderkwast


## Naming conventions
Valkey has a long history of inconsistent naming conventions. Generally follow the style of the surrounding code, but you can also always use the following conventions for variable and structure names:

- Variable names: `snake_case` or all lower case (e.g. `valkeyobject` or `valkey_object`)
Copy link
Member

Choose a reason for hiding this comment

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

can we converge on snake_case exclusively, for the new code?

- Function names: `camelCase` or `namespace_camelCase` (e.g. `createObjectList` or `networking_createObjectList`).
- Macros: `UPPER_CASE` (e.g. `DICT_CREATE`)
- Structures: `camelCase` (e.g. `user`)

## Licensing information
When creating new source code files, use the following snippet to indicate the license:
```
/*
* Copyright (c) Valkey Contributors
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
*/
```

If you are making material changes to a file that has a different license at the top, also add the above license snippet.
There isn't a well defined test for what is considered a material change, but a good rule of thumb is if it's more than 100 lines of code.

## Test coverage
Valkey uses two types of tests: unit and integration tests.
All contributions should include a test of some form.

Unit tests are present in the `src/unit` directory, and are intended to test individual structures or files.
For example, most changes to datastructures should include corresponding unit tests.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, most changes to datastructures should include corresponding unit tests.
For example, most changes to data structures should include corresponding unit tests.


Integration tests are located in the `tests/` directory, and are intended to test end-to-end functionality.
Adding new commands should come with corresponding integration tests.
When writing cluster mode tests, do not use the legacy `tests/cluster` framework, which has been deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

can we add a documentation section as well? new commands, configs, non-trivial features should come with documentation? blogs are also encouraged for major features, such as the 1M QPS, the hash table rewrite

## Best practices
1. Avoid adding configuration when a feature can be fully controlled by heuristics.
We want Valkey to work correctly out of the box without much tuning.
Configurations can be added to provide additional tuning of features.
When the workload characteristics can't be inferred or imply a tradeoff (CPU vs memory), then provide a configuration.
2. Try to limit the number of lines changed in a PR when possible.
We do a lot of backporting as a project, and the more lines changed, the higher the chance of having to resolve merge conflicts.