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

Type defs for custom validation, fixes. #122

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Type defs for custom validation, fixes. #122

merged 3 commits into from
Apr 1, 2020

Conversation

FFKL
Copy link
Contributor

@FFKL FFKL commented Mar 30, 2020

Hello! I've improve some type definitions.

  • CompilationRule, Context, CheckerFunction, CompilationFunction for custom rule definitions (directly in schema or by add method).
  • RuleCustomInline upd: I think, generic isn't necesssary. Input type for validation - unknown.
  • ValidationSchema fix: ability to use of root-level schema fields. For example - min: 1
  • ValidationError fix: type changed to message name, message - string for custom message
  • compile, validate methods fix: now it can be any type - not only object.

@coveralls
Copy link

coveralls commented Mar 30, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling c4cb8a0 on FFKL:type-def-upd into 5a699b9 on icebob:master.

@FFKL
Copy link
Contributor Author

FFKL commented Mar 30, 2020

@icebob in source code i found sanitized property for validation object - { sanitized: boolean, source: string}. What is it used for?

@icebob
Copy link
Owner

icebob commented Mar 31, 2020

sanitized & source are used internally only.

@icebob
Copy link
Owner

icebob commented Mar 31, 2020

@akazakou could you review, plz?

Copy link
Contributor

@akazakou akazakou left a comment

Choose a reason for hiding this comment

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

@FFKL thank you for your PR. It mostly looks good to me, but I have a few questions

index.d.ts Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
customs: { [ruleName: string]: { schema: RuleCustom, messages: MessagesType } }
}

type CheckerFunction = (value: unknown, schema: ValidationSchema, path: string, parent: object | null, context: Context) => true | ValidationError[];
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckerFunction should have a generic type for value parameter.

Suggested change
type CheckerFunction = (value: unknown, schema: ValidationSchema, path: string, parent: object | null, context: Context) => true | ValidationError[];
type CheckerFunction<T = unknown> = (value: T, schema: ValidationSchema, path: string, parent: object | null, context: Context) => true | ValidationError[];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same case as in the previous conversation

Copy link
Contributor

Choose a reason for hiding this comment

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

Guess it can be resolved by that comment #122 (comment)

index.d.ts Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
@FFKL
Copy link
Contributor Author

FFKL commented Mar 31, 2020

sanitized & source are used internally only.

Maybe sanitized but not source - https://github.com/icebob/fastest-validator#custom-validator

Copy link
Contributor

@akazakou akazakou left a comment

Choose a reason for hiding this comment

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

@icebob @FFKL I do not have additional questions and that PR looks good to me 🚀

@icebob
Copy link
Owner

icebob commented Apr 1, 2020

@akazakou @FFKL so can I merge?

@FFKL
Copy link
Contributor Author

FFKL commented Apr 1, 2020

@akazakou @FFKL so can I merge?

I think - yes)

Copy link
Owner

@icebob icebob left a comment

Choose a reason for hiding this comment

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

Thanks to both of you!

@icebob icebob merged commit 41d162d into icebob:master Apr 1, 2020
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.

4 participants