Skip to content

Move FieldValidator Validate() methods to be be on a pointer value #110

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

Closed
smyrman opened this issue Jul 9, 2017 · 5 comments
Closed

Move FieldValidator Validate() methods to be be on a pointer value #110

smyrman opened this issue Jul 9, 2017 · 5 comments

Comments

@smyrman
Copy link
Collaborator

smyrman commented Jul 9, 2017

We use quite some documentation-real estate to explain that schema.FeildValidators should be set using pointers so that any (potential) schema.Compiler interface (maybe renamed after #100 ) can be detected and called. THere is also code catching this mis-usage and giving a runtime error.

However, it's generally better to fail faster, and it would be pretty easy to make this a compile-time error by moving the Validate method for most FieldValidator implementations to be on a pointer value:

/ String validates string based values
type String struct {
	...
}

// Compile compiles and validate regexp if any.
func (v *String) Compile() (err error) {
	...
}

// Validate validates and normalize string based value.
func (v *String) Validate(value interface{}) (interface{}, error) {
	...
}

This also means that the run-time checks can be removed!

PS! Some types, such as maps and slices, do not require neither Compiler nor Validate to be on a pointer value due to how these types work.

@smyrman smyrman changed the title Move FieldValidator Validate() method to be be on a pointer value. Move FieldValidator Validate() methods to be be on a pointer value Jul 9, 2017
@smyrman
Copy link
Collaborator Author

smyrman commented Jul 9, 2017

@rs: Created a new label type for this issue. Hope that's OK.

@rs
Copy link
Owner

rs commented Jul 9, 2017

np, what should we use it for?

@rs
Copy link
Owner

rs commented Jul 9, 2017

Good idea btw. We might want to keep the check to make sure custom types are use properly even if their methodes aren’t on pointers. Does it make sense?

@smyrman
Copy link
Collaborator Author

smyrman commented Jul 9, 2017

We might want to keep the check to make sure custom types are use properly even if their methodes aren’t on pointers. Does it make sense?

Depends if we can make the tests right. Sometimes It's not thecnically necessary to use a pointer type, such as map and slice types. When there is no Compile, it's also technically no reason to make the field a pointer.

np, what should we use it for?

My thinking was to label issues that are not bugs, but could help with the stability of projects using rest-layer.

E.g.: earlier errors, safer and more robust implementations off existing features.

@smyrman
Copy link
Collaborator Author

smyrman commented Sep 5, 2018

I want to close this issue in favor of #194.

@smyrman smyrman closed this as completed Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants