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: [#416] Support add custom filter for validation #516

Merged
merged 8 commits into from
Jun 27, 2024

Conversation

kkumar-gcc
Copy link
Member

πŸ“‘ Description

Closes goravel/goravel#416

βœ… Checks

  • Added test cases for my code

@kkumar-gcc kkumar-gcc closed this Jun 25, 2024
@kkumar-gcc kkumar-gcc reopened this Jun 25, 2024
@kkumar-gcc kkumar-gcc requested a review from a team June 25, 2024 05:38
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 69.98%. Comparing base (0ad5442) to head (ce15251).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   69.90%   69.98%   +0.07%     
==========================================
  Files         180      180              
  Lines       11000    11025      +25     
==========================================
+ Hits         7690     7716      +26     
+ Misses       2739     2738       -1     
  Partials      571      571              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

devhaozi
devhaozi previously approved these changes Jun 25, 2024
@@ -5,10 +5,16 @@ type Option func(map[string]any)
type Validation interface {
// Make create a new validator instance.
Make(data any, rules map[string]string, options ...Option) (Validator, error)
// AddFilter add the custom filter.
AddFilter(name string, filterFunc any) Validation
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to define a filter interface and pass it here. Like the rule:

image

Add a command to create a filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, it's not a good idea since the filter function can vary.
image

Copy link
Contributor

@hwbrzzl hwbrzzl Jun 25, 2024

Choose a reason for hiding this comment

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

Maybe:

type Filter interface {
	// Signature set the unique signature of the filter.
	Signature() string
	// Passes determine if the validation rule passes.
	Handle() func(val any) (any, error)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, it's not a good idea since the filter function can vary.

We can only support one style: func(val any) (any, error)

Copy link
Member Author

@kkumar-gcc kkumar-gcc Jun 25, 2024

Choose a reason for hiding this comment

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

A developer can also create a filter function like this:

func Default(val string, def ...string) string {
    ...
}

which can be used like this:

map[string]string{
   "name" : "default:krishan"
}
// assume registered as "default"

So, let's not add constraints on developers since gookit/validate provides this feature.

Copy link
Member Author

@kkumar-gcc kkumar-gcc Jun 25, 2024

Choose a reason for hiding this comment

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

Maybe:

type Filter interface {
	// Signature set the unique signature of the filter.
	Signature() string
	// Passes determine if the validation rule passes.
	Handle() func(val any) (any, error)
}

we can change it to ?

type Filter interface {
	// Signature set the unique signature of the filter.
	Signature() string
	// Passes determine if the validation rule passes.
	Handle() any // will mention in docs that any can only be two type of function
}

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to add a new Filter method in the FormRequest:

image

Users can use the Filter feature through the Filter method, instead of set a tag in the struct:

image

The way of set a tag in the struct is inconsistent with the rule, so I suggest optimize it. cc @devhaozi

So we need to fdmovd the logic in goravel/gin and goravel/fiber and add some new logic:

image

@kkumar-gcc kkumar-gcc closed this Jun 26, 2024
@kkumar-gcc kkumar-gcc reopened this Jun 26, 2024
@kkumar-gcc kkumar-gcc requested a review from a team June 26, 2024 15:05
devhaozi
devhaozi previously approved these changes Jun 26, 2024
Copy link
Member

@devhaozi devhaozi left a comment

Choose a reason for hiding this comment

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

LGTM

hwbrzzl
hwbrzzl previously approved these changes Jun 27, 2024
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Great PR πŸ‘ Please add some integration tests in goravel/example

// Signature sets the unique signature of the filter.
Signature() string

// Handle defines the filter function to apply.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great

@kkumar-gcc kkumar-gcc dismissed stale reviews from hwbrzzl and devhaozi via 783a586 June 27, 2024 08:09
@kkumar-gcc kkumar-gcc enabled auto-merge (squash) June 27, 2024 08:13
@kkumar-gcc kkumar-gcc merged commit 612c188 into master Jun 27, 2024
10 checks passed
@kkumar-gcc kkumar-gcc deleted the kkumar-gcc/#416 branch June 27, 2024 08:24
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.

✨ [Feature] Support add custom filter for validation
3 participants