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

Implement the custom format validator #51

Merged
merged 3 commits into from
Sep 25, 2019

Conversation

arjan
Copy link
Contributor

@arjan arjan commented Sep 11, 2019

See #50

@jonasschmidt
Copy link
Owner

Sounds like a great idea! I will have a look at the PR later today and give some feedback.

As there is no room in the error struct for custom messages, we can
have the function just return a boolean.
@arjan
Copy link
Contributor Author

arjan commented Sep 11, 2019

The potential issue I see with this solution that it's using the Application environment. Meaning that when two independent parts of the project use a custom format validator, they need to ensure that they reach agreement of the callback module to use.

A better solution would be to add an extra context argument to the validate call, which can carry this validator configuration all the way into the validation. This would also allow to put custom values in the context which then could be used in the validation callback. However, this would be a very large change, wiring that context argument through every function call.. so my proposal is maybe not ideal, but quite pragmatic.

@jonasschmidt
Copy link
Owner

jonasschmidt commented Sep 12, 2019

Yes, good point. Another option would be to put the format validator in the %ExJsonSchema.Schema.Root{} struct that contains the resolved schema. One could argue that it's logically a part of the schema anyway, so it would fit there. That struct would only have to be passed to ExJsonSchema.Validator.Format.validate to make it available there.

@jonasschmidt
Copy link
Owner

So a great approach would be to (optionally) pass configuration options to ExJsonSchema.Schema.resolve and put them in the schema struct. The current logic could also stay there as a fallback, so the user has the choice of whether to define the format validator globally or per schema.

@coveralls
Copy link

coveralls commented Sep 16, 2019

Coverage Status

Coverage increased (+0.01%) to 98.673% when pulling 9b9c758 on botsquad:custom-format-validator into 134b61a on jonasschmidt:master.

@arjan
Copy link
Contributor Author

arjan commented Sep 25, 2019

I force-pushed to remove my last commit which made the return value "nicer", so now the validator should return a list of errors again.

I agree that passing down a format validator inside the struct would be nice, but it's additional to this PR, I'll create a ticket for it after this is merged.

@jonasschmidt
Copy link
Owner

Ah, sorry, that was a misunderstanding. I prefer the version you had in the end, where the format validator function just returns a boolean. I should have phrased that comment a bit more clearly.

Any comments on the idea of putting the custom format validator in the schema struct?

@arjan
Copy link
Contributor Author

arjan commented Sep 25, 2019

Oh, hah, oops :-) Git reflog ftw, I've pushed again.

About the custom format validator, I think it could be stored in the %Root{} struct under an options key or whatever. And then pass this options down into the ExJsonSchema.Validator.validate_aspect function, which forwards it to each validator.

@arjan
Copy link
Contributor Author

arjan commented Sep 25, 2019

But like I said maybe it's better to do that in a separate PR.

@jonasschmidt
Copy link
Owner

Ok, that makes sense. I will merge and release the current state. Thank you for the excellent work 👍

@jonasschmidt jonasschmidt merged commit 0d6e04f into jonasschmidt:master Sep 25, 2019
@arjan
Copy link
Contributor Author

arjan commented Sep 25, 2019

Thanks!

@jonasschmidt
Copy link
Owner

Just released on Hex as well as version 0.7.0.

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.

None yet

3 participants