valkey-io / valkey-doc

Other
17 stars 25 forks source link

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

Closed stockholmux closed 3 days ago

stockholmux commented 1 month ago

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 commented 1 month ago

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: https://github.com/redis/redis/pull/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 commented 1 month ago

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

stockholmux commented 1 month ago

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 commented 1 month ago

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!