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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schema can be inherited #127

Merged
merged 3 commits into from Jan 17, 2017
Merged

Conversation

her0e1c1
Copy link
Contributor

@her0e1c1 her0e1c1 commented Dec 5, 2016

Hi nice to meet you!
I love your python library, which is small but powerful 馃槃

But I have a case to convert data before validating.
When I tried override validate method of Schema, I found the Schema in And and Or class need to be changed.

I am happy if you merge this commit or give me some advice 馃槃

Thanks!

- Added schema keyword argument to `__init__` of `And` and `Or` to override `validate` of `Schema`

- Replace `Schema` with `self.__class__` in `validate` of `Schema`
self._error = kw.get('error')
self._schema = kw.get('schema', Schema)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what is the rationale for this? Could you add a short comment explaining why someone can pass the class here? Why shouldn't every subclass do self.__class__ instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! I think there is a bit problem if you use Schema directly because it's public class but it can not be inherited.

About this code self.__class__, I think it's good enough to use it in only Schema. I mean about other code, it's good to pass inherited Schema class instead

@codecov-io
Copy link

Current coverage is 100% (diff: 100%)

Merging #127 into master will not change coverage

@@           master   #127   diff @@
====================================
  Files           1      1          
  Lines         178    180     +2   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits          178    180     +2   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update 0d83627...cc525da

@skorokithakis
Copy link
Collaborator

Hmm, I'd like to merge this, but checks are failing. I guess it's just Pypy's problem, and that the PR is fine, is that correct?

@her0e1c1
Copy link
Contributor Author

@skorokithakis I'm glad to here that! So please check it by myself later :D

@her0e1c1
Copy link
Contributor Author

I don't know the reason and it could help you but I ran this command on this branch

docker run --rm -it -v `pwd`:/t -w /t pypy:3 bash -c 'pip install tox && tox -e pypy3'

I got test success

@skorokithakis
Copy link
Collaborator

Looks like it's because pip doesn't work with pypy3 (it reports itself as Python 3.2, which pip doesn't work with). Merging, thank you!

@skorokithakis skorokithakis merged commit 9c6d082 into keleshev:master Jan 17, 2017
@dAnjou
Copy link

dAnjou commented Mar 10, 2017

@her0e1c1 When I look at your test I'm wondering whether it would have been easier to simply do this:

from schema import Schema, Use


def convert(data):
    if isinstance(data, int):
        return data + 1
    return data

s = {
    'k': Use(convert),
    'd': {
        'k': Use(convert),
        'l': [{'l': [Use(convert)]}]
    }
}
v = {'k': 1, 'd': {'k': 2, 'l': [{'l': [3, 4, 5]}]}}
d = Schema(s).validate(v)

assert d['k'] == 2
assert d['d']['k'] == 3
assert d['d']['l'][0]['l'] == [4, 5, 6]

@her0e1c1
Copy link
Contributor Author

Hi @dAnjou :) Yeah the test case can be more simple.
But my test purpose is to inherit. I mean, if there is not my commits, the test would fail.

And in my case (sorry suddenly), I want to convert sqlalchemy model class to dict before validating.
I think In the case, you can not use Use only

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.

None yet

4 participants