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

Document the commands JSON files #403

Merged
merged 5 commits into from May 2, 2024

Conversation

zuiderkwast
Copy link
Contributor

These JSON files were originally not intended to be used directly, since they
contain internals and some fiels like "acl_categories" that are not the final
ACL categories. (Valkey will apply some implicit rules to compute the final ACL
categories.) However, people see JSON files and use them directly anyway.

So it's better to document them.

In a later PR, we can get rid of all implicit ACL categories and instead populate them explicitly in the JSON files. Then, we'll add a validation (e.g. in generate-command-code.py) that the implied categories are set.

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.36%. Comparing base (68ca258) to head (d647a68).

❗ Current head d647a68 differs from pull request most recent head 257e46c. Consider uploading reports for the commit 257e46c to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #403      +/-   ##
============================================
- Coverage     68.43%   68.36%   -0.08%     
============================================
  Files           109      108       -1     
  Lines         61671    61563     -108     
============================================
- Hits          42207    42085     -122     
- Misses        19464    19478      +14     

see 14 files with indirect coverage changes

src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
src/commands/README.md Outdated Show resolved Hide resolved
Thanks Binbin!

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>

Co-authored-by: Binbin <binloveplay1314@qq.com>
Copy link
Contributor

@stockholmux stockholmux left a comment

Choose a reason for hiding this comment

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

Great to see this. A couple of small issues.

src/commands/README.md Show resolved Hide resolved
src/commands/README.md Show resolved Hide resolved
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
@zuiderkwast zuiderkwast merged commit d1de349 into valkey-io:unstable May 2, 2024
14 checks passed
@zuiderkwast zuiderkwast deleted the document-commands-json branch May 2, 2024 15:02
PingXie pushed a commit to PingXie/valkey that referenced this pull request May 2, 2024
These JSON files were originally not intended to be used directly, since
they contain internals and some fiels like "acl_categories" that are not
the final ACL categories. (Valkey will apply some implicit rules to
compute the final ACL categories.) However, people see JSON files
and use them directly anyway.

So it's better to document them.

In a later PR, we can get rid of all implicit ACL categories and instead
populate them explicitly in the JSON files. Then, we'll add a validation
(e.g. in generate-command-code.py) that the implied categories are set.

---------

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: Binbin <binloveplay1314@qq.com>
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

Successfully merging this pull request may close these issues.

None yet

3 participants