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

First CLASS validators #89

Closed
wants to merge 7 commits into from
Closed

First CLASS validators #89

wants to merge 7 commits into from

Conversation

philtay
Copy link
Contributor

@philtay philtay commented Dec 8, 2014

I'd appreciate an early code review. Then I'll write the unit tests. Spotted and fixed a bug in the email validator. Added Function and NoneOf validators. With these two I consider the validate module complete and ready for battle!

@philtay
Copy link
Contributor Author

philtay commented Dec 9, 2014

The unit tests for all of the class-based validators are in the commit above.
It includes the previously missing tests for URL and Email validators.
It also expands and makes more robust several other tests.
In my opinion, the class-based validators are in a mergeable state now.

@sloria
Copy link
Member

sloria commented Dec 9, 2014

Thank you for your contribution.

This PR contains unrelated changes. Can you please separate this into 3 PRs?

  • Decimal field
  • Convertion to class-based validators
  • Two added validators (it is fine if this one is dependent on the 2nd PR)

Once those are sent, I'd be glad to review them!

@philtay
Copy link
Contributor Author

philtay commented Dec 9, 2014

@sloria if it's not a problem for you I'd prefer to keep them in a single pr. it's more easy for me and the same for you. these unrelated changes are all in different files. you should not have any problem in reviewing them separately. thank you

@sloria
Copy link
Member

sloria commented Dec 9, 2014

I would much prefer to review them as separate PRs. Having the separate branches not only helps mergeability, but also adds useful historical information to the commit log.

Since the changes are in separate files, it should be straightforward to do:

git checkout -b decimal origin/dev
# add decimal field; commit; push
git checkout -b class-validators origin/dev
# add validators; commit; push
git checkout -b new-validators validators
# add Function and NoneOf; commit; push

@philtay
Copy link
Contributor Author

philtay commented Dec 10, 2014

Ok. I'll open the PRs. Just a few minutes.

@sloria sloria closed this Dec 11, 2014
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.

2 participants