valkey-io / valkey-doc

Other
17 stars 25 forks source link

`/topics/cluster-tutorial.md` code blocks have inline numbers and potential issue with code #141

Closed stockholmux closed 1 day ago

stockholmux commented 1 month ago

In the pre-publishing review (#91), I found the following issues in cluster-tutorial.md

1 require './cluster' 2 3 if ARGV.length != 2

The line numbers are embedded directly into the code block (w/o syntax highlighting), making it unusable if someone tried to copy/paste out of it. (adding ,linenos to the backtick annotation will ensure that it does have line numbers that don't get copy/pasted).

Additionally, this file is a duplicate of https://github.com/antirez/redis-rb-cluster/blob/master/example.rb. The repo antirez/redis-rb-cluster doesn't have a license and a few of the files do have a license headers but not example.rb, so I'm a bit mixed on it's inclusion. I think the best course of action here would be remove the copy/paste and just link to the file (and potentially ask for license clarification on the whole repo).

Note: If not for backward compatibility, the Valkey project no longer uses the word slave. Unfortunately in this command the word slave is part of the protocol, so we'll be able to remove such occurrences only when this API will be naturally deprecated.

I've noticed this disclaimer on a few pages, it might be more useful to have this at the top of the document rather than the absolute bottom, but just a thought.

zuiderkwast commented 2 weeks ago

I'm fixing the code block, deleting the line numbers.

Regarding example.rb, he is the author of this code so he is (or was) allowed to publish it under multiple licenses. The copy he published in these docs are under our docs license, so I think we're safe in this regard.

Regarding the note about slave: I'd say it depends on how important the note is. A disclaimer like the one you added like "this page is under review and may be delete" belongs at the top, but this note is more like a footnote to me. The content of the text itself is already edited to exclude the word slave except where it's necessary for API compatibility reasons, as the note says. (In man pages, there's normally a NOTES section near the bottom of the page.)

We should update that note some time, and the content of all pages, to stop using "master" too, but that's a future improvement.

zuiderkwast commented 2 weeks ago

Regarding using redis-rb-cluster for the example, technically a non-open source library, is not perfect, but it serves well as pseudo code. That's how I'd assume most developers read this. I opened #147 for replacing these examples.

This page is very useful as an introduction so I hope this is not a blocking point.

stockholmux commented 2 weeks ago

@zuiderkwast Well, looking at the git blame, I don't think that Salvatore actually added the example.rb (or at least the version represented in the docs). Feels dubious.

zuiderkwast commented 2 weeks ago

That commit just made a minor chsnge to the example. It added a way to specify IP on the command line. It just added line 3

   3  if ARGV.length != 2

And

   8  else
   9      startup_nodes = [
  10          {:host => ARGV[0], :port => ARGV[1].to_i}
  11      ]
  12  end

Apart from that, only the line numbers are changed, afaict.