HOT-FIX: CRITICAL - fixed set command optional arguments and added tests to set command#1508
Conversation
avifenesh
force-pushed
the
HotFix/SetWithOption
branch
2 times, most recently
from
b3c51a0 to
6ce9cbc
Compare
|
@acarbonetto Please have a look on the tests, see if you think this is a good structure. |
Yury-Fridlyand
left a comment
Yury-Fridlyand
left a comment
There was a problem hiding this comment.
Consider adding UT/IT for all options in options
acarbonetto
left a comment
acarbonetto
left a comment
There was a problem hiding this comment.
A couple of comments. I would suggest splitting up your larger test into smaller tests to avoid data conflicts.
@Yury-Fridlyand - Can you explain a bit more? I'm not sure i understand what is the different from the existing tests to what you offer. |
avifenesh
force-pushed
the
HotFix/SetWithOption
branch
from
6ce9cbc to
95a5116
Compare
|
In java client we added UT where we test that all combinations of options are properly converted to command args. |
avifenesh
changed the title
avifenesh
force-pushed
the
HotFix/SetWithOption
branch
2 times, most recently
from
3f6f795 to
0f99d73
Compare
node/src/Commands.ts
Outdated
| args.push("KEEPTTL"); | ||
| } else if (options.expiry?.type === "seconds") { | ||
| args.push("EX " + options.expiry.count); | ||
| args.push("EX", String(options.expiry.count)); |
There was a problem hiding this comment.
minor: use .toString() instead of String(), it should be guaranteed at this point that count is a number and has the toString built in function
There was a problem hiding this comment.
Actually we have already a check before that count is an int and we dont need recheck it. To me String() is more visually appealing, but if you find it important, i can change.
Normally the decision to take to_string instead of string is when the object you run it on has an override in your class/object of to_string,.
There was a problem hiding this comment.
https://courses.bigbinaryacademy.com/learn-javascript/string-methods/tostring-vs-string-method/
I read this blog and it seems like String is for any type of data and toString is to convention for number type, and also it will match what we already have in other commands, which is relevant to have the same conversion to string all over as we're going to change it all to byte-convertor, and it would make it easier to find
There was a problem hiding this comment.
toString works for object, bool, strings and maybe more. But okay, I'll change. It's not a critical issue.
avifenesh
force-pushed
the
HotFix/SetWithOption
branch
from
0f99d73 to
e28fcb2
Compare
avifenesh
force-pushed
the
HotFix/SetWithOption
branch
from
e28fcb2 to
663ad92
Compare
…sts to set command (#1508) fixed set command optional arguments and added tests to set command
* fixed CD for last changes * Change to using json file for our matrix * Remove issues with many package.json variation in deployed package * minor fixes * error looking fix * fixed adding package.json-e in mac * added fix to avoid typo in tag to release rc version as real one * fixed matrix of tests * added sudo to ts install * updated to run tests also in real release * updated README to include supported versions * return setup node since tests break when doing it manually * comments fix * HOT-FIX: CRITICAL - fixed set command optional arguments and added tests to set command (#1508) fixed set command optional arguments and added tests to set command * added 0.4.1 bug fix to changelog * fix platform_matrix generating * fix npm notice err * fix usage of global tsc * fix usage of global tsc -p
…sts to set command (valkey-io#1508) fixed set command optional arguments and added tests to set command
* fixed CD for last changes * Change to using json file for our matrix * Remove issues with many package.json variation in deployed package * minor fixes * error looking fix * fixed adding package.json-e in mac * added fix to avoid typo in tag to release rc version as real one * fixed matrix of tests * added sudo to ts install * updated to run tests also in real release * updated README to include supported versions * return setup node since tests break when doing it manually * comments fix * HOT-FIX: CRITICAL - fixed set command optional arguments and added tests to set command (valkey-io#1508) fixed set command optional arguments and added tests to set command * added 0.4.1 bug fix to changelog * fix platform_matrix generating * fix npm notice err * fix usage of global tsc * fix usage of global tsc -p
5 participants
#1507
The set command options are not functioning properly.
We found out about it thanks to @Ealenn open an issue.
In order to avoid the repetition of similar bugs, the PR incorporates a fix and a large quantity of tests.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.