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

/topics/modules-intro.md has missing syntax highlighting and missing/work in progress. #121

Closed
stockholmux opened this issue May 29, 2024 · 4 comments · Fixed by #153
Closed

Comments

@stockholmux
Copy link
Member

stockholmux commented May 29, 2024

In pre-publishing review (#91), I found the following issues:

This document is about an alpha version of Valkey modules. API, functionalities and other details may change in the future.

Should this document be updated to represent the current modules API or removed?

Code blocks don't use backticks nor do they have a language defined for syntax highlighting

Set type API
Work in progress.

and

Iterating aggregated values
Work in progress.

This can't be right, probably due to this document being written before the API was done.

Hash type API
Documentation missing, please refer to the top comments inside module.c for the following functions:

and

Writing commands compatible with Valkey Cluster
Documentation missing, please check the following functions inside module.c:

This needs to be completed.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented May 29, 2024

Do you want to delete this document? I think it is a very good introduction to modules. It explain the basic stuff like how to create a command.

This document is about an alpha version of Valkey modules. API, functionalities and other details may change in the future.

Should this document be updated to represent the current modules API or removed?

Just delete this note. The API didn't change much so this page is a good intro to the module API IMO.

Code blocks don't use backticks nor do they have a language defined for syntax highlighting

That's not an error. Indentation four spaces is valid code block syntax. (It's the original markdown, which didn't include triple backticks. https://daringfireball.net/projects/markdown/syntax#precode) Syntax highlighting is nice but the lack of it shouldn't block useful content, should it?

Set type API
Work in progress.

This is accurate actually. It has been in the TODO list since forever but there was some activity last year: redis/redis#12874

Iterating aggregated values
Work in progress.

This can't be right, probably due to this document being written before the API was done.

Partially right. There's a sorted set iterator and a stream iterator. Lists can be iterated using indexes. For sets and hashes, there's not yet an iterator API. There's a scan API for hash, set and sorted set though so I'm not sure an iterator API is necessary for those.

Hash type API
Documentation missing, please refer to the top comments inside module.c for the following functions:

This must be from before we had the generated module API ref. I think we should delete these API overviews per key type, since they're outdated and the updated content is in the module API ref.

Writing commands compatible with Valkey Cluster
Documentation missing, please check the following functions inside module.c:

This needs to be completed.

... or easier, we can link to these functions in the module API ref, where they're documented.

The module-api-ref.md is generated from those comments in module.c.

@zuiderkwast
Copy link
Contributor

The implementation is not much less incomplete than what the docs suggest. Here's an overview of what's missing: redis/redis#8157

@stockholmux
Copy link
Member Author

Okay, sounds like it just needs some cleanup then. It looked like a point-in-time, historical thing like some of the other docs.

WRT indent code blocks vs backticks:

Yeah, not really an error but a concern, and certainly not something I would ever block on. If we have the ability to do syntax highlight we should probably move blocks to the highlighter (albeit, a low priority). It's just nicer for the reader and costs nothing.

Otherwise, the highlighter is linebreak aware (where possible based on language) and makes it display more nicely on narrow devices. Otherwise, consistency in how we handle this is desirable.

@zuiderkwast
Copy link
Contributor

Yes, I certainly agree many things can be improved, but my main concern is that the docs are not yet up. It's up on Redis websites since forever. Things like "Salvatore, the creator of Valkey" needs to go, like any legal issues, but other improvements are just something we should do incrementally IMO. Getting it all reviewed is a really good start!

zuiderkwast added a commit to zuiderkwast/valkey-doc that referenced this issue Jul 3, 2024
…hlighing, valkey-io#121

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
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 a pull request may close this issue.

2 participants