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

Fixes to resp2_replies.json and resp3_replies.json #202

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

gmbnomis
Copy link
Contributor

@gmbnomis gmbnomis commented Dec 21, 2024

Fix keys, add missing commands/options, replace Redis/master

Fix keys, add missing commands/options, replace Redis/master

Signed-off-by: Simon Baatz <[email protected]>
@stockholmux
Copy link
Member

stockholmux commented Dec 23, 2024

@zuiderkwast Refresh my memory: these reply JSON files are somehow generated?

I think @gmbnomis' effort here is worthy but I'm not sure of the right way to get it into the docs if it is generated.

@zuiderkwast
Copy link
Contributor

I don't think they are generated. The bulk of it is markdown text, not types. IIRC they were extracted from the command markdown files some time in the past.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Very good, thanks!

Did you test this by generating man pages to review the result?

I opened this for the website: valkey-io/valkey-io.github.io#185

A problem with not having this on the website is that contibutors forget to update these files, so they become more and more outdated.

I would actually like to put this content back into each command's markdown file under a Reply heading, to eliminare the need for special handling. Looking at the history, it was in each markdown file earlier.

"SENTINEL IS MASTER-DOWN-BY-ADDR": [],
"SENTINEL MASTER": [
"[Map reply](../topics/protocol.md#maps): The state and info of the specified master."
"SENTINEL IS PRIMARY-DOWN-BY-ADDR": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Both names exist for backward compatibility. We should probably duplicate these for both the primary and master subcommands.

Copy link
Contributor Author

@gmbnomis gmbnomis Dec 24, 2024

Choose a reason for hiding this comment

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

I just did "robotic" replacements here, but actually:

  1. The sentinel commands appear only in the resp3 file
  2. The sentinel command documentation lives outside of the regular command documentation anyway: https://valkey.io/topics/sentinel/#sentinel-api

I tend to remove the Sentinel commands from here. But I can also duplicate these both for master/primary and resp2/resp3 respectively if we intend to make the sentinel commands "first class citizens" in the future.

@gmbnomis
Copy link
Contributor Author

gmbnomis commented Dec 24, 2024

Did you test this by generating man pages to review the result?

I looked at some of the web pages and some man pages (especially ZSCAN and HSCAN).

I opened this for the website: valkey-io/valkey-io.github.io#185

valkey-io/valkey-io.github.io#183 😀

I would actually like to put this content back into each command's markdown file under a Reply heading, to eliminare the need for special handling. Looking at the history, it was in each markdown file earlier.

Yes, putting them back into the command's markdown file might be a good idea longer term (or generate them automatically from the command JSONs).

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Awesome, then let's merge this asap. We can improve more in the future.

@zuiderkwast
Copy link
Contributor

(or generate them automatically from the command JSONs)

The descriptive text is not in the commnad json though. You mean we should move it there?

@zuiderkwast zuiderkwast merged commit 07968b7 into valkey-io:main Dec 24, 2024
2 checks passed
@gmbnomis gmbnomis deleted the resp_fixes branch December 24, 2024 18:55
@gmbnomis
Copy link
Contributor Author

The descriptive text is not in the commnad json though. You mean we should move it there?

My original intention was to eliminate duplication between the per-command JSON files and the RESP2/RESP3 files. Ideally, the JSON files would contain sufficient information to auto-generate the reply section(s) in the documentation, with the possibility of adding more descriptive text in this repository.

However, upon closer examination, this approach is flawed: the per-command JSON files lack adequate type information for RESP3 responses. Given this limitation, it seems we may need to accept the duplication.

@madolson
Copy link
Member

madolson commented Jan 9, 2025

@gmbnomis @zuiderkwast How are we keeping the newly updated resp2 and resp3 files in sync with the main repo? When trying to deploy valkey-io/valkey-io.github.io#183, it was already out of sync with the main repo. Do we want to make a change on valkey.io so that you can't add commands without defining all the necessary components in the documentation maybe?

@gmbnomis
Copy link
Contributor Author

gmbnomis commented Jan 9, 2025

@madolson My bad, I did not implement a fallback to make sure that no command is missing from the reply files and did not add that later.

Do we want to make a change on valkey.io so that you can't add commands without defining all the necessary components in the documentation maybe?

I think having a check to ensure that there is an entry for each command in the reply files (it may just be [] for command groups) makes a lot of sense. This won't ensure that the reply files aren't outdated, but we could at least ensure that authors have to think about documenting replies when adding a new command.

@zuiderkwast
Copy link
Contributor

I think having a check to ensure that there is an entry for each command in the reply files (it may just be [] for command groups) makes a lot of sense.

Yes, good idea! This can be a check just within the doc repo. For each command markdown file, check that there's an entry in reply json files. We can add it in the doc repo CI.

Do we want to make a change on valkey.io so that you can't add commands without defining all the necessary components in the documentation maybe?

Apart from the above, it's hard to keep the repos in sync, so I would prefer that we make this logic resilient to differences, rather than blocking publishing. If the JSON file or the .md file is missing, we can skip that command rather than crashing. And skip the reply section if the reply json is missing.

To build man pages for 8.0.2 (which is missing some commands from unstable) I had to make a similar change to man page generation: #210

@madolson
Copy link
Member

madolson commented Jan 9, 2025

Apart from the above, it's hard to keep the repos in sync, so I would prefer that we make this logic resilient to differences, rather than blocking publishing. If the JSON file or the .md file is missing, we can skip that command rather than crashing. And skip the reply section if the reply json is missing.

It wouldn't be that hard to make the docs a superset of the main project. It would just force us add docs first and then merge changes into the main repository. It's a forcing function to keep stuff in sync.

My other concern is that we might just forget and never add the values to the RESP json files.

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.

4 participants