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

feat(ui-utils): add overload signatures for validateNumber function in validationUtils #2029

Merged
merged 3 commits into from
May 16, 2024

Conversation

skamril
Copy link
Member

@skamril skamril commented May 14, 2024

Modification to allow validateNumber function to support currying and composition:

rules={{
  validate: (v) => validateNumber(v, { min: 0 }),
}}

=>

rules={{
  validate: validateNumber({ min: 0 }),
}}

rules={{
  validate: (v: string) => validateNumber(Number(v))
}}

=>

rules={{
  validate: R.compose(validateNumber, Number)
}}

@skamril skamril self-assigned this May 14, 2024
@skamril skamril requested a review from hdinia May 14, 2024 10:41
@skamril skamril force-pushed the feature/validation-utils branch 2 times, most recently from ef4d97f to 78d7171 Compare May 15, 2024 11:59
Copy link
Member

@hdinia hdinia left a comment

Choose a reason for hiding this comment

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

Nice work on integrating currying into the validateNumber it makes the usage more flexible and clean. However, I'm curious about the use of function composition here. Could you explain a bit more about why we're mixing it with the validation logic, is that a need you have to use it on another PR?

thanks for clarifying

Comment on lines 7 to 12
interface NumberValidationOptions {
min?: number;
max?: number;
}

interface ValidationOptions extends NumberValidationOptions {
Copy link
Member

Choose a reason for hiding this comment

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

Consolidating shared properties like min and max into a single interface eliminates redundancy but keep in mind the context, here their serve different purposes. I think we should keep them separate to simplify future additions and flexibility

Copy link
Member Author

@skamril skamril May 16, 2024

Choose a reason for hiding this comment

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

I forgot to commit a fix for the length message sorry.
I change this.


const value = valueOrOpts;

if (isFinite(value)) {
Copy link
Member

Choose a reason for hiding this comment

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

you're returning an error message when the number is valid (finite). This should be changed to check for the opposite condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

hdinia
hdinia previously approved these changes May 16, 2024
@skamril
Copy link
Member Author

skamril commented May 16, 2024

Nice work on integrating currying into the validateNumber it makes the usage more flexible and clean. However, I'm curious about the use of function composition here. Could you explain a bit more about why we're mixing it with the validation logic, is that a need you have to use it on another PR?

Composition here it's just to show the double avantage to have this signature in general.
In fact the purpose of this PR is to add an example of signature overload in the project.

@skamril skamril merged commit d51a5b0 into dev May 16, 2024
5 checks passed
@skamril skamril deleted the feature/validation-utils branch May 16, 2024 11:47
@laurent-laporte-pro laurent-laporte-pro added this to the v2.17.1 milestone May 24, 2024
@laurent-laporte-pro laurent-laporte-pro mentioned this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants