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

Json type #214

Merged
merged 9 commits into from Jul 10, 2018

Conversation

2 participants
@oldPadavan
Contributor

oldPadavan commented Jun 29, 2018

Implement #195

oldPadavan added some commits Jun 29, 2018

@codecov

This comment has been minimized.

codecov bot commented Jun 29, 2018

Codecov Report

Merging #214 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #214   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     11           
  Lines        1327   1365   +38     
  Branches      245    249    +4     
=====================================
+ Hits         1327   1365   +38
class Json(Generic[JsonObj]): # defined not in types.py to avoid circular imports
pass

This comment has been minimized.

@samuelcolvin

samuelcolvin Jun 29, 2018

Owner

Move this to types.py and implement get_validators

This comment has been minimized.

@samuelcolvin

samuelcolvin Jun 29, 2018

Owner

I also don't htink you need JsonObj = TypeVar('JsonObj', Dict[str, Any], List) etc.

This comment has been minimized.

@oldPadavan

oldPadavan Jul 6, 2018

Contributor

I've implemented JsonObj = TypeVar('JsonObj', Dict[str, Any], List) in order to be possible to write Json[List[int]], without it I was getting exceptions like JsonObj = TypeVar('JsonObj', Dict[str, Any], List) or TypeError: typing.Union[typing.List, typing.Dict] is not a generic class. So I implemented Json class a a subclass of typing.Generic. If You know other way, please let me know, because I didn't find it

@@ -181,6 +184,9 @@ def _populate_sub_fields(self):
elif issubclass(origin, Set):
self.type_ = self.type_.__args__[0]
self.shape = Shape.SET
elif issubclass(origin, Json):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jun 29, 2018

Owner

I don't think you need to make any changes in fields.py

This comment has been minimized.

@oldPadavan

oldPadavan Jul 6, 2018

Contributor

The reason behind changes in fields.py is that Json[List] had __origin__ attribute, thus in _populate_sub_fields I was getting AssertionError after origin was compared against List, Dict etc.

class JsonError(PydanticValueError):
msg_template = "{json_str} is not valid JSON and can't be decoded"

This comment has been minimized.

@samuelcolvin

samuelcolvin Jun 29, 2018

Owner

I don't think we should include the original string in the error message, it could be very long.

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Jul 6, 2018

Ok, sounds like I need to look into it more and getting a better understanding.

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Jul 7, 2018

ok, I would use something like

class JsonMeta(type):
    def __getitem__(self, t):
        return type('JsonWrapperValue', (JsonWrapper,), {'inner_type': t})


class Json(metaclass=JsonMeta):

    @classmethod
    def get_validators(cls):
        yield str_validator
        yield json_validator

class JsonWrapper:
    __slots__ = 'inner_type',

Naked use of Json eg.

class Foobar(BaseModel):
    x: Json

won't need anything special.

Parameterised use of Json eg.

class Foobar(BaseModel):
    x: Json[List[int]]

will need some changes to fields.py, basically you'll just need

self.type_ = self.type_.inner_type
self.parse_json = True

Then parse json before continuing with normal validation.

@oldPadavan

This comment has been minimized.

Contributor

oldPadavan commented Jul 10, 2018

I don't really get why deploy/netlify fails. Log says it failed to clone repository:

10:55:36 AM: git clone git@github.com:samuelcolvin/pydantic
10:56:10 AM: Error cloning repository: git@github.com:samuelcolvin/pydantic
10:56:10 AM: Failing build: Failed to prepare repo
10:56:10 AM: failed during stage 'preparing repo': exit status 128

@samuelcolvin

Looking good, just a bunch of small things to fix.

Netlify was just problems with their service, I restarted the build and it's working now.

class JsonError(PydanticValueError):
msg_template = "Not valid JSON provided - it can't be decoded"

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

Just Invalid JSON would be more succinct and clear.

loc = (loc,)
loc = loc if isinstance(loc, tuple) else (loc, )
v, error = self._validate_json(v, loc)

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

Please can you move the if self.parse_json to here.

so

if self.parse_json:
    v, error = self._validate_json(v, loc)
    if error:
        ...

This comment has been minimized.

@oldPadavan

oldPadavan Jul 10, 2018

Contributor

I moved checking of self.parse_json to self._validate_json in order to skip from flake8 warning pydantic/fields.py:244:5: C901 'Field.validate' is too complex (11)

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

Thought that might be the case, just add # noqa: C901 (ignore complexity) to the end of the function definition.

if self.parse_json:
try:
return json.loads(v), None
except (json.JSONDecodeError, TypeError):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

you can use ValueError instead of son.JSONDecodeError. Do we actually need TypeError?

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

and below.

This comment has been minimized.

@oldPadavan

oldPadavan Jul 10, 2018

Contributor

I included TypeError in pydantic/fields.py to handle these cases:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.6/json/__init__.py", line 348, in loads
    'not {!r}'.format(s.__class__.__name__))
TypeError: the JSON object must be str, bytes or bytearray, not 'int'```

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

ok, but that should raise a descendent if TypeError not ValueError, best to use a separate except

@@ -239,6 +251,7 @@ def arbitrary_type_validator(v) -> type_:
(time, [parse_time]),
(timedelta, [parse_duration]),
(JsonWrapper, [str_validator, json_validator]),

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

This doesn't make any sense, can just be removed.

@@ -13,6 +14,10 @@
NoneType = type(None)
class JsonWrapper:

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

this can be moved into types.py

@@ -213,6 +218,13 @@ def path_exists_validator(v) -> Path:
return v
def json_validator(v: str):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

clearer if this is moved into a validate class method on Json

@@ -154,11 +158,14 @@ def schema(self, by_alias=True):
def _populate_sub_fields(self):
# typing interface is horrible, we have to do some ugly checks
if inspect.isclass(self.type_) and issubclass(self.type_, JsonWrapper):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

just use isinstance(self.type_, type) and issubclass(self.type_, JsonWrapper)

assert JsonModel(json_obj=json.dumps(obj)).dict() == {'json_obj': obj}
class JsonDetailedModel(BaseModel):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

can you move the class definition into each test since all the field setup is done here and can fail.

class JsonModel(BaseModel):
json_obj: Json
obj = {'a': 1, 'b': [2, 3]}

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

confused to dump then load. Better to use raw JSON strings in the tests

'{"a": 1, "b": [2, 3]}'

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

and below.

@samuelcolvin

Can you also add a new line to HISTORY.rst and fix coverage.

def validate(cls, v: str):
try:
return json.loads(v)
except json.JSONDecodeError:

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

use ValueError

return v, None
try:
return Json.validate(v), None
except (ValueError, TypeError) as exc:

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

split ValueError and TypeError as below.

class JsonNotStrError(PydanticTypeError):
code = 'json_not_str'

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

name JsonTypeError and change code to just 'json' which will result in the final code being type_error.json

class JsonNotStrError(PydanticTypeError):
code = 'json_not_str'
msg_template = "JSON object must be str, bytes or bytearray"

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

single quotes for strings please, and above.

This comment has been minimized.

@samuelcolvin

samuelcolvin Jul 10, 2018

Owner

also can you add a test for pssing in bytes.

oldPadavan added some commits Jul 10, 2018

@samuelcolvin samuelcolvin merged commit c31b8d6 into samuelcolvin:master Jul 10, 2018

3 checks passed

codecov/project 100% (+0%) compared to 03517e4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Jul 10, 2018

awesome, thank you.

We also need to add docs for this. Would you be able to work on that in a separate PR? Otherwise, I'll do it when I get a chance.

@oldPadavan

This comment has been minimized.

Contributor

oldPadavan commented Jul 11, 2018

Should I add it to Exotic Types section or somewhere else?

@samuelcolvin

This comment has been minimized.

Owner

samuelcolvin commented Jul 11, 2018

I would say best if it was a new section after Exotic Types, or perhaps a new subsection of exotic types.

@oldPadavan oldPadavan referenced this pull request Jul 11, 2018

Merged

Docs for json type #228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment