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 Equality between schemas #210

Merged
merged 3 commits into from
Sep 24, 2016

Conversation

tusharmakkar08
Copy link
Collaborator

Fixes #208

@tusharmakkar08
Copy link
Collaborator Author

@alecthomas : Since __repr__ was the combination of object and extras and schemas, __eq__ was giving wrong in cases Required etc were present. Hence I proceeded with making a __str__ for Schema. Now it'll satisfy cases mentioned by @AndreaCrotti .

@tusharmakkar08 tusharmakkar08 merged commit 987fd98 into alecthomas:master Sep 24, 2016
@alecthomas
Copy link
Owner

Ok... I feel like this could use some tests though.

@tusharmakkar08
Copy link
Collaborator Author

Aren't doctests sufficient ?

@alecthomas
Copy link
Owner

I was thinking more tests for deeply nested schemas, using things like Any() etc.

@tusharmakkar08
Copy link
Collaborator Author

Hmm..
I have couple of concerns:

  • For scenarios like this:

    schema = Schema(Any(None, int))
    schema1 = Schema(Any(int, None))

Do we need to have schema==schema1 as True? If yes, then the current logic won't work.

  • I'll add more tests using Nested schemas and stuff. I will create a separate PR for the same, instead of reverting this PR. Is it fine?

Thanks

@alecthomas
Copy link
Owner

Yeah, I'm not sure about Any (and possibly others) which is kind of why I brought it up. It's probably fine as-is though, for 99% of cases.

@tusharmakkar08
Copy link
Collaborator Author

Cool 👍

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