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

Create method to Validator that accepts data argument. #267

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

glennpratt
Copy link

Currently the Validator class requires data be passed into the constructor, though it doesn't do anything important with it then.

In my case, I'm validating in a server and I see no reason to parse the schema over and over for different inputs, especially since it uses two mutexes in the initialize method.

Let me know if I missed something and Validator simply must accept data at construction.

Is #validate_data(data) the way to go, or should I change the signature of #validate to take an optional argument? The former seemed like a clean change with little API change.

@glennpratt
Copy link
Author

I wrote this little script to look for any obvious threading issues.

https://gist.github.com/glennpratt/bf254a896c9b0f15b812

No errors on 2.1.6 and JRuby.

@RST-J
Copy link
Contributor

RST-J commented Oct 3, 2015

To me this looks fine. 👍

@glennpratt
Copy link
Author

This needs work, have issues with shared @errors instance var in testing.

@glennpratt
Copy link
Author

OK, pushed a commit that should fix that issue.

Need more tests around that apparently.

@glennpratt
Copy link
Author

Note: This PR is sparse to help decide if this is a good direction to move it.

If it is, I'll update usage, documentation and tests.

So, do people think this is the right direction?

(Thanks @RST-J)

@jpmckinney
Copy link
Contributor

Yes, this is the right direction. Re-initializing a schema everytime you validate a record is extremely slow. The schema should be initialized once, and then used to validate any number of records.

@iainbeeston
Copy link
Contributor

Yes I agree with the direction completely. I would actually change the signature of the validate method. We already have a 3.x branch for exactly this kind of backwards incompatible change.

Once you're happy with it could open a new pull request on the 3.x branch?

@iainbeeston iainbeeston added this to the v3 milestone Dec 23, 2015
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