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: validateIf for validation options #1579

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

aoi-umi
Copy link

@aoi-umi aoi-umi commented Mar 11, 2022

Description

Add validateIf for validation options.
If you want to validate that condition by object, you can use validation validateIf.

ValidateIf will skip all other decorators, validation validateIf only effect the decorator that use this option

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

references #1455

Franco1312
Franco1312 previously approved these changes Mar 28, 2022
@braaar
Copy link
Member

braaar commented Dec 5, 2022

I think I prefer the skip or skipIf name for this option. I think validateIf is a bit confusing, since you can get it mixed up with the ValidateIf decorator.

@braaar
Copy link
Member

braaar commented Dec 5, 2022

Handy feature, though!

Copy link
Member

@braaar braaar left a comment

Choose a reason for hiding this comment

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

I think this is a good proposal, but I think the function should be normalized to match the predicate function used in ValidateIf.

This is how I see it being the most useful:

@Max(5, { validateIf: (o) => Boolean(o.someOtherProperty) })

I am now of the opinion that validateIf might be better than skip / skipIf.

@braaar
Copy link
Member

braaar commented Dec 15, 2023

See my recent comment in #1721

Copy link
Member

@braaar braaar left a comment

Choose a reason for hiding this comment

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

As discussed in #1721, I think this decorator should have the same signature as ValidateIf.

The signature (value: any, validationArguments: ValidationArguments) => boolean; makes it unnecessarily cumbersome to do simple stuff like:
@IsUUID({ validateIf: o => o.type === "something" })

@aoi-umi
Copy link
Author

aoi-umi commented Jan 24, 2024

updated @braaar

@kevit-kelvin-mandlik
Copy link

@braaar Please merge this feature I am waiting for so long...

@kevit-kelvin-mandlik
Copy link

@braaar @aoi-umi What are you guys waiting upon this? Can we please move forward with this?

@aoi-umi
Copy link
Author

aoi-umi commented Mar 14, 2024

@braaar @aoi-umi What are you guys waiting upon this? Can we please move forward with this?

haha, i don't know either

@braaar
Copy link
Member

braaar commented Mar 19, 2024

We are discussing this feature internally right now. Hold on just a little longer, @kevit-kelvin-mandlik and @aoi-umi

@kevit-kelvin-mandlik
Copy link

@braaar Are we good to go on this??

@yasaichi
Copy link

Any updates on this PR? I've been looking forward to using this feature for a quite long time!

@braaar
Copy link
Member

braaar commented Sep 6, 2024

Hi again. Could I ask that you rewrite the docs and tests so that they more clearly illustrate the usefulness of this validation option? Personally I find the example in this PR a bit hard to parse. What does this actually mean in practice, you know?

      validateIf: (obj: MyClass, value) => {
        return !obj.someOtherProperty || obj.someOtherProperty === 'min';
      },

I feel like that makes a poor case for this validation option. Adding a new feature should make people go "Nice! I like this!" rather than "Huh?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants