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

Fix shutdown syntax hint, following intention #116

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

LiiNen
Copy link
Contributor

@LiiNen LiiNen commented Apr 1, 2024

This change is the thing I suggested to redis when it was BSD, and thankfully was reviewed by @zuiderkwast

Problem

valkey-cli, for now, is not fancy I thought.
The intention was that we cannot use 'ABORT' option with others - this only can be used alone.
But for now the hints show us that we can use it together, though this actually leads syntax error.
image
image
image

solved

simply fix hint
image

ABORT will not be appeared when others

image

other options will not be appeared when using ABORT

image

Note

Of course, I did refer to previous commit, but didn't fully reuse it.
I edit json, build, and check it certainly again :)
Thanks valkey!
image

p.s. when testing it, found the left word 'Redis' from first line in above img. Renaming redis to valkey is already in lots of PR, so I had checked previous PRs, and found PR that covered most of it, but not covering this phrase: 'Redis version=255.255.255'. Left suggestion comment in #88

@LiiNen LiiNen changed the title Fix shutdown syntax Fix shutdown syntax hint, following intention Apr 1, 2024
@LiiNen LiiNen marked this pull request as draft April 1, 2024 13:54
@LiiNen LiiNen force-pushed the fix-shutdown-hint branch from d11fbff to e6b4040 Compare April 1, 2024 13:55
@LiiNen LiiNen marked this pull request as ready for review April 1, 2024 13:56
@LiiNen
Copy link
Contributor Author

LiiNen commented Apr 2, 2024

Spellcheck seems weird...?

@zuiderkwast
Copy link
Contributor

Spellcheck was changed in #72. I don't know why it fails now if it didn't fail when we introduced the new tool. @jayvdb do you want to take a look?

@jayvdb
Copy link
Contributor

jayvdb commented Apr 2, 2024

typos has been improved. It has found new errors, and some new false positives.
I'll do a PR

@zuiderkwast
Copy link
Contributor

I merged #139. @LiiNen can you rebase?

LiiNen added 2 commits April 2, 2024 22:00
Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
@LiiNen LiiNen force-pushed the fix-shutdown-hint branch from e6b4040 to 86dce25 Compare April 2, 2024 13:00
@LiiNen
Copy link
Contributor Author

LiiNen commented Apr 2, 2024

force pushed it with rebase

@zuiderkwast zuiderkwast merged commit ba8bda9 into valkey-io:unstable Apr 2, 2024
15 checks passed
@zuiderkwast
Copy link
Contributor

Thanks @LiiNen! It is also OK to add merge commits instead of rebase. We squash-merge it in the end anyway. Note that the merge-commit also needs the signed-off-by header though.

@LiiNen
Copy link
Contributor Author

LiiNen commented Apr 2, 2024

Got it thanks,
I am now considering; add alias commit='commit -s' lol

@zuiderkwast zuiderkwast added the polish typos, style, etc label Apr 2, 2024
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Change syntax hint from

    SHUTDOWN [NOSAVE | SAVE] [NOW] [FORCE] [ABORT]

to

    SHUTDOWN [[NOSAVE | SAVE] [NOW] [FORCE] | ABORT]

It's not that important for docs, but the latter is preferred for valkey-cli's automatic syntax hints.

Signed-off-by: LiiNen <kjeonghoon065@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
polish typos, style, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants