fix: "wrong number of arguments" must be in lower case #438
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Leonardo Mello <[email protected]>
dranikpg
reviewed
Oct 26, 2022
There was a problem hiding this comment.
Hi!
- Its great you pointed out the neighbouring functions as well. If you run
SET k v EX -1on redis, you'll see that it returnsERR invalid expire time in 'set' command. In case withUnknownSubCmdthe command is returned in original form. SoInvalidExpireTimecan be changed as well - See my comment. No need for a separate function
- There is no real facade_test, its a helper library for other tests
src/facade/facade.cc
Outdated
Comment on lines
54
to
55
| string lcmd(cmd); | ||
| absl::AsciiStrToLower(&lcmd); |
There was a problem hiding this comment.
Abseil already has a one-liner for your use case https://github.com/abseil/abseil-cpp/blob/2fc358dab079c220feb81f75b5a366f8d1464cd4/absl/strings/ascii.h#L170-L175
There was a problem hiding this comment.
Oh I haven't seem that. Fixed!
And also lowered the command on the InvalidExpireTime
Signed-off-by: Leonardo Mello <[email protected]>
Signed-off-by: Leonardo Mello <[email protected]>
|
@lsvmello Do you mind merging all the commits into a single one on your side? I could do it too but then the commit won't track to you, imho. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Leonardo Mello [email protected]
This fixes #390.
Since the print pointed out just the "wrong number of arguments" message I lowered the command just for the
WrongNumArgsErrorfunction.Should the command also be lowered on
InvalidExpireTimeandUnknownSubCmd? If that's the case then is it ok to create aToLowerfunction forstring_viewtypes on thefacade.ccor is it better to put it on another file?I couldn't execute the
facade_testalso, it compiles but doesn't create any executable.First open source PR ever so if there is any improvement just say it and I'll be glad work on it.