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

Create development guide #1558

Open
madolson opened this issue Jan 14, 2025 · 7 comments
Open

Create development guide #1558

madolson opened this issue Jan 14, 2025 · 7 comments

Comments

@madolson
Copy link
Member

I'm just starting an issue to dump all of the conversations we've had about getting started as a developer on Valkey. At some point we should commit them to a markdown, but just wanted to start taking notes since I've seen a few examples of best practices/naming conventions/common pitfalls.

General coding guidelines.

  1. Most of the style comments are enforced by clang format.
  2. C style comments /* comment */ can be used for both single and multi-line comments. C++ comments // can only be used for single line comments.

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 (valkeyobject or valkey_object)
  • Function names: camelCase or namespace_camelCase (createObjectList or networking_createObjectList).
  • Macros: UPPER_CASE (DICT_CREATE)
  • structures: camelCase (user)

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. When the workload characteristics can't be inferred or imply a tradeoff (CPU vs memory), then provide a configuration.
@sarthakaggarwal97
Copy link
Contributor

We can also include the minimum test / functional coverage required, and how can we write / run TCL tests (maybe)?

@ranshid
Copy link
Member

ranshid commented Jan 15, 2025

Some things that came to mind:

  1. use of static functions when possible and internal to the object
  2. architecture specific definitions: where to place them/ which architectures should be supported/ which compiler flavors etc...
  3. regarding configurations, maybe we should help programers decide when feature control should be supported by config or debug command (eg tests)
  4. makefile/Cmake conventions (?)

@pieturin
Copy link
Contributor

We should specify that we want to keep line lengths to max 120 characters.

@enjoy-binbin
Copy link
Member

We should specify that we want to keep line lengths to max 120 characters.

90 chars? i think 120 is still quite long and hard to read the long sentence for me (bad at reading)

@enjoy-binbin
Copy link
Member

also, i think we should mention the backport thing? How do we do the backport now? Do we do the backport when a PR got merged like #1575? Or we just mark a label and when the release date come, someone from the trusted list do the local cherry-pick thing and push they together?

@hpatro
Copy link
Collaborator

hpatro commented Jan 17, 2025

also, i think we should mention the backport thing? How do we do the backport now? Do we do the backport when a PR got merged like #1575? Or we just mark a label and when the release date come, someone from the trusted list do the local cherry-pick thing and push they together?

That's a good question. I think it's much better if the developer who fixed/submitted the PR to unstable can also backport it to the release branches as they can deal with the merge conflicts better and then the trusted list of people can review and merge it. What are your thoughts @enjoy-binbin ?

@sarthakaggarwal97
Copy link
Contributor

sarthakaggarwal97 commented Jan 17, 2025

I think it's much better if the developer who fixed/submitted the PR to unstable can also backport it to the release branches as they can deal with the merge conflicts better

I agree. We can also have a bot that can raise the backport PRs (using cherry-picks, so the commit is technically already reviewed), incase there is no conflict if the backport label is present?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants