Skip to content

Conversation

AmirAllayarovSofteq
Copy link
Contributor

No description provided.

@AmirAllayarovSofteq AmirAllayarovSofteq added the issue Something isn't working label Oct 17, 2022
@AmirAllayarovSofteq AmirAllayarovSofteq self-assigned this Oct 17, 2022
@CLAassistant
Copy link

CLAassistant commented Oct 17, 2022

CLA assistant check
All committers have signed the CLA.

@AmirAllayarovSofteq AmirAllayarovSofteq changed the title #RI-3612-initial commit #RI-3612-zset infinity score Oct 19, 2022
Copy link
Collaborator

@ArtemHoruzhenko ArtemHoruzhenko left a comment

Choose a reason for hiding this comment

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

Bakend looks good. I've added few comments. It will be nice to address them but not necessary in case if we must move forward asap.

}

defaultMessage(args: ValidationArguments) {
return `${args.property || 'field'} must be a string or a number`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

must be a string? "hello world" will be not a valid value here...

BrowserToolZSetCommands.ZScore,
[keyName, member],
);
const formattedScore = isNaN(parseFloat(score)) ? String(score) : parseFloat(score);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this to some function to not duplicate logic everywhere?

while (reply.length) {
const member = reply.splice(0, 2);
const score = isNaN(parseFloat(member[1])) ? String(member[1]) : parseFloat(member[1]);
result.push(plainToClass(ZSetMemberDto, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are using "plainToClass" here. Maybe it is better to create own transformer and isNaN(parseFloat(member[1])) ? String(member[1]) : parseFloat(member[1]); will be done automatically inside?
P.S. See comment above

name: Joi.string().required(),
// todo: allow(true) - is incorrect but will be transformed to number by BE. Investigate/fix it
score: Joi.number().required().allow(true),
score: Joi.number().required().allow('inf', '-inf').label('.score'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is better to create own type here the same way as for redisString so it will look like Joi.zSetScore().required() or something similar

members: Joi.array().items(Joi.object().keys({
name: JoiRedisString.required(),
score: Joi.number().required(),
score: Joi.number().required().allow('inf', '-inf'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same

members: Joi.array().items(Joi.object().keys({
name: JoiRedisString.required(),
score: Joi.number().required(),
score: Joi.number().required().allow('inf', '-inf'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

the same

@Type(() => Number)
score: number;
@isZSetScore()
score: number | 'inf' | '-inf';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to create a type (like type ZSetScore = ...) and use it everywhere

@AlenaSY AlenaSY merged commit b38a31a into main Oct 19, 2022
@AlenaSY AlenaSY deleted the bugfix/RI-3612_infinity_score_zset branch October 19, 2022 14:29
@ViktarStarastsenka ViktarStarastsenka removed the issue Something isn't working label Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants