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

Doc fixes part 3, various topics pages #197

Merged
merged 19 commits into from
Dec 20, 2024

Conversation

Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
topics/introduction.md Outdated Show resolved Hide resolved
Signed-off-by: Viktor Söderqvist <[email protected]>
Copy link
Member

@stockholmux stockholmux left a comment

Choose a reason for hiding this comment

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

Happy to see this move forward.

topics/admin.md Outdated Show resolved Hide resolved
topics/command-tips.md Outdated Show resolved Hide resolved
topics/faq.md Outdated Show resolved Hide resolved
Such script can perform conditional updates across multiple keys, possibly combining several different data types.

Using `EVAL` requires that the application sends the entire script for execution every time.
Because this results in network and script compilation overheads, Valkey provides an optimization in the form of the `EVALSHA` command. By first calling `SCRIPT LOAD` to obtain the script's SHA1, the application can invoke it repeatedly afterward with its digest alone.
Because this results in network and script compilation overheads, Valkey provides an optimization in the form of the [`EVALSHA`](../commands/evalsha.md) command.
Copy link
Member

Choose a reason for hiding this comment

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

This is more of a question for the style guide (which we still need to make).

Should every instance of a command need to be linked or only the first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the first? WDYT?

Here, I added it only on the first occurrence (at least that was the intention).

On another website where this content has been used, every occurrence of all commands were automatically linked. If we want that, then I think we should do that too. I'm not sure I like that though. Do you?

Copy link
Member

Choose a reason for hiding this comment

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

I'm honestly split @zuiderkwast. I thought that was what you were going with here. I do think we need to start linking topics and command reference in some way.

Pros for link-on-first-command only:

  1. Less obnoxious to do manually.
  2. Less likely of a clash between a arguments that look like commands (e.g. argument GET in BITFIELD linked to command GET).
  3. Overly linked pages have some negative SEO impact as it dilutes the flow between pages

Cons for link-on-first-command only:

  1. Entry point for documentation might not be linear, so if someone links to a header, the actual link is above the header.
  2. Moving text around could potentially change whichever is the "first" link

(Also: I could easily automate this from the website using JS but it doesn't help us SEO wise and it doesn't help man pages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want automatic linking? In that case, it should be done in the pre-processing phase (zola), not in the browser.

The bang-syntax is all there just to indicate that it shouldn't be auto-linked, for example the GET option to the SET command is marked as !GET, which we'd remove in the pre-processing phase. If we're going this way, then we should not delete the bangs.

For man pages, auto-linking doesn't make much sense, since there are no links in man pages. It's better to list them at the bottom of the page in a SEE ALSO section. I could do that in the preprocessing too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we have automatic linking, let's keep these explicit links? It's not more than one link to each page here.

topics/functions-intro.md Outdated Show resolved Hide resolved
topics/functions-intro.md Outdated Show resolved Hide resolved
e = s[0..-(bits+1)]+("1"*bits)
puts "ZRANGE myindex [#{s} [#{e} BYLEX"
}
```ruby
Copy link
Member

Choose a reason for hiding this comment

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

Have we tested this in a modern Ruby?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I don't intend to spend time on that. I'd rather delete it, or the entire page. 😆

Copy link
Member

Choose a reason for hiding this comment

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

Fair.

I've heard of actions that will test snippets for syntax errors and stuff like this. Might be worth setting up.

With regard to this particular snippet it was written 9 years ago and someone modified it 2 years ago to remove deprecated Ruby stuff according to git blame. That said, I'm fine leaving it as-is, but I think the problem (generally) still exists.

Copy link
Contributor Author

@zuiderkwast zuiderkwast Dec 12, 2024

Choose a reason for hiding this comment

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

OK, let's leave this one as is then.

The interactive valkey-cli is very cool and maybe we can integrate it for the valkey-cli command session examples, but I think it would be a lot of work and maintenance to have interactive code example in all languages. I'd rather just put in a disclaimer or change it to pseudo-code, because the purpose is to serve as an example for understanding, not for copy-paste.

Copy link
Member

Choose a reason for hiding this comment

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

I will add I don't think people really use indexes like are outlined in this document. It's very fragile to keep all of these different indexes in sync, and if it breaks the system is extremely hard to recover from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want to delete this page?

I didn't consider that. I just tried to fix Kyle's comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madolson if you want to delete the indexing page, I suggest we do that in another PR. We'd also need to remove links to it from other pages, etc.

topics/installation.md Outdated Show resolved Hide resolved
topics/faq.md Outdated Show resolved Hide resolved
Signed-off-by: Viktor Söderqvist <[email protected]>
topics/data-types.md Outdated Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Dec 18, 2024

Addressed the comments. Is it OK to merge?

@stockholmux
Copy link
Member

@zuiderkwast merge!

@zuiderkwast zuiderkwast merged commit 8b9f09c into valkey-io:main Dec 20, 2024
2 checks passed
@zuiderkwast zuiderkwast deleted the doc-fixes-part-3 branch December 20, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment