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/cluster-tutorial.md code blocks have inline numbers and potential issue with code #141

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

Comments

@stockholmux
Copy link
Member

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.

@stockholmux stockholmux changed the title /topics/cluster-tutorial.md code blocks have line numbers /topics/cluster-tutorial.md code blocks have inline numbers and potential issue with code May 31, 2024
@zuiderkwast
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

stockholmux commented Jun 20, 2024

@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
Copy link
Contributor

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.

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

* Remove junk line numbers in code examples and incorrect output in the valkey-cli prompt in examples in in cluster-tutorial.
* Remove reference to Redis OSS 3 and revert an occurrence of "adapterd to Redis coding style", since that's what this old code was.

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