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

Adding SomeOf validator #314

Merged
merged 5 commits into from
Dec 7, 2017
Merged

Adding SomeOf validator #314

merged 5 commits into from
Dec 7, 2017

Conversation

guyarad
Copy link
Contributor

@guyarad guyarad commented Dec 3, 2017

The SomeOf validator can verify that some of validators are met, or optionally at most number of validators.

@coveralls
Copy link

coveralls commented Dec 3, 2017

Coverage Status

Coverage increased (+0.03%) to 95.411% when pulling 267f197 on guyarad:master into 9204e83 on alecthomas:master.

class SomeOf(object):
"""Value must pass at least some validations, determined by the given parameter

The output of each validator is passed as input to the next.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

description will be improved in the next update of this PR (I'm sure there will be so I'm waiting with pushing it 😄 )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Haha.. Let's not rely on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will push it asap

@guyarad
Copy link
Contributor Author

guyarad commented Dec 5, 2017

Is this repository not active anymore?

@tusharmakkar08
Copy link
Collaborator

Hey @guyarad

I was busy over the weekends. Will review it and get back to you. Before my review, please fix the failing tests.

Thanks.

@guyarad
Copy link
Contributor Author

guyarad commented Dec 5, 2017

@tusharmakkar08 no rush - just wanted to know how active we are 😊

the failing tests aren't mine. they seem to exist after the last commit to master.

Copy link
Collaborator

@tusharmakkar08 tusharmakkar08 left a comment

Choose a reason for hiding this comment

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

Please look into the comments.

"""

def __init__(self, min_valid, validators, max_valid=None, **kwargs):
self.min_valid = min_valid or 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why min_valid is not a default keyword argument while max_valid is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I only needed "minimum valid schemas" and just added "max valid" for completeness. So for me, min_valid was always needed. We can put them both as optional, and assert that either of them has a value (otherwise, what's the point of this validator).

Sounds good?

@tusharmakkar08
Copy link
Collaborator

Hey @guyarad

Thanks for pointing out the reason behind failing tests. Have created an issue for the same: #315 .

I have glanced over the PR. Have a couple of comments. Let's resolve them and get this merged.

Thanks.

@coveralls
Copy link

coveralls commented Dec 6, 2017

Coverage Status

Coverage increased (+0.04%) to 95.415% when pulling ef97726 on guyarad:master into 9204e83 on alecthomas:master.

.python-version Outdated
3.6.3
3.3.6
3.4.7
3.5.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file was accidentally added because I was working with pyenv which proved to be very useful for testing locally with multiple python versions.

I was going to remove it, but then thought it would be a good to have such convention as repository users, to use pyenv. your call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alecthomas What do you think about adding this to the package?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove this for now and create an issue where we will continue this discussion.

validator('3A34SDEF5')


def test_SomeOf_max_validation():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a test for checking the assert statement too using AssertionError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.python-version Outdated
3.6.3
3.3.6
3.4.7
3.5.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

@alecthomas What do you think about adding this to the package?

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.05%) to 95.424% when pulling 5e13321 on guyarad:master into 9204e83 on alecthomas:master.

@coveralls
Copy link

coveralls commented Dec 7, 2017

Coverage Status

Coverage increased (+0.05%) to 95.424% when pulling 63eba67 on guyarad:master into 9204e83 on alecthomas:master.

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.

3 participants