-
Notifications
You must be signed in to change notification settings - Fork 40
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
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]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
There was a problem hiding this 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.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- Less obnoxious to do manually.
- Less likely of a clash between a arguments that look like commands (e.g. argument
GET
in BITFIELD linked to commandGET
). - Overly linked pages have some negative SEO impact as it dilutes the flow between pages
Cons for link-on-first-command only:
- Entry point for documentation might not be linear, so if someone links to a header, the actual link is above the header.
- 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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
e = s[0..-(bits+1)]+("1"*bits) | ||
puts "ZRANGE myindex [#{s} [#{e} BYLEX" | ||
} | ||
```ruby |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😆
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Viktor Söderqvist <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Addressed the comments. Is it OK to merge? |
@zuiderkwast merge! |
/topics/introduction.md
needs updated language about 'support' #111/topics/installation.md
references exploit venerable below Redis 3.2 and has instructions forinit.d
#108/topics/indexing.md
needs code block and language notation #107/topics/hashes.md
links to wrong project #106/topics/functions-intro.md
has bad formatting, improper framing #104/topics/faq.md
has outdated info #103/topics/eval-intro.md
has several issues #102/topics/data-types.md
has references to 'community supported modules' #100/topics/command-tips.md
Text indicates that command tips not yet implemented #99/topics/admin.md
has outdated info #96/topics/get-started.md
is sparse #105