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

Fix const validations #794

Merged
merged 1 commit into from Sep 17, 2019
Merged

Conversation

@hmvp
Copy link
Contributor

hmvp commented Sep 6, 2019

This fixes #620

NOTICE: pydantic is currently pushing towards V1 release,
see issue 576. Changes not required to release version 1
may be be delayed until after version 1 is released. If your PR is a bugfix for v0.32, please base off and target the v0.32.x branch.

Change Summary

Related issue number

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.rst file added describing change
    (see changes/README.md for details)
@hmvp hmvp force-pushed the hmvp:fix_const_validations branch 2 times, most recently from 4a6b5cf to f964990 Sep 6, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 6, 2019

Codecov Report

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

@@          Coverage Diff           @@
##           master   #794    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          17     17            
  Lines        2984   2847   -137     
  Branches      580    559    -21     
======================================
- Hits         2984   2847   -137
Impacted Files Coverage Δ
pydantic/fields.py 100% <100%> (ø) ⬆️
pydantic/error_wrappers.py 100% <100%> (ø) ⬆️
pydantic/types.py 100% <0%> (ø) ⬆️
pydantic/errors.py 100% <0%> (ø) ⬆️
pydantic/typing.py 100% <0%> (ø) ⬆️
pydantic/networks.py 100% <0%> (ø) ⬆️
pydantic/validators.py 100% <0%> (ø) ⬆️
pydantic/schema.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b467da...e6d6389. Read the comment docs.

@hmvp

This comment has been minimized.

Copy link
Contributor Author

hmvp commented Sep 6, 2019

Not sure if you wan't this in master or 0.32 so I submitted both

Copy link
Owner

samuelcolvin left a comment

looks like a good start, please can you also fix coverage.

pydantic/fields.py Outdated Show resolved Hide resolved
pydantic/fields.py Outdated Show resolved Hide resolved
HISTORY.rst Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Sep 6, 2019

does this also resolve #793?

@hmvp hmvp force-pushed the hmvp:fix_const_validations branch from f964990 to fb08c18 Sep 7, 2019
@hmvp

This comment has been minimized.

Copy link
Contributor Author

hmvp commented Sep 7, 2019

does this also resolve #793?

No, unfortunately it doesn't, and I don't really have a clue how to fix it. However it might not be a big issue for us since we initialize the constant fields with json compatible dicts instead of submodel instances

@hmvp hmvp force-pushed the hmvp:fix_const_validations branch from fb08c18 to 43152ee Sep 7, 2019
@hmvp

This comment has been minimized.

Copy link
Contributor Author

hmvp commented Sep 7, 2019

does this also resolve #793?

No, unfortunately it doesn't, and I don't really have a clue how to fix it. However it might not be a big issue for us since we initialize the constant fields with json compatible dicts instead of submodel instances

Ok changed it up a bit more and now it should work for #793. At least in the most obvious cases.

It still fails if you initialize the default value with a BaseModel instance instead of a dict.

@hmvp hmvp force-pushed the hmvp:fix_const_validations branch from 43152ee to 9260975 Sep 7, 2019
Copy link
Owner

samuelcolvin left a comment

Looking good to me, just a few small things to change.

@tiangolo, please could you review this since you did most of the original work on schema.


m = Model()
assert m.a == [SubModel(b=1), SubModel(b=2), SubModel(b=3)]
assert m.b == [SubModel(b=4), SubModel(b=5), SubModel(b=6)]

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 9, 2019

Owner

would it also help to add m.schema() == {...} here?

This comment has been minimized.

Copy link
@hmvp

hmvp Sep 17, 2019

Author Contributor

Added, So this is interesting:

It seems that adding SubModel instances as default breaks in a lot of ways: Also the schema is now not json serializable, similar to the error as per #793 .

It might be a good idea to just document or add a warning of some sorts for that case and consider that unsupported. Of course not all use cases need json serializability and some use cases might want to set the default using a SubModel instance.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 17, 2019

Owner

please create an issue about this.

try:
# Fails
Form(field1={'field': 1}, field2=[{'field': 1}])
except ValidationError as e:

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 9, 2019

Owner

use pytest.raises(ValidationError) as exc_info:.... exc_info.e.json()

This comment has been minimized.

Copy link
@hmvp

hmvp Sep 17, 2019

Author Contributor

Fixed

Form(field1={'field': 1}, field2=[{'field': 1}])
except ValidationError as e:
# This should not raise an Json error
assert json.dumps(e.json())

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 9, 2019

Owner

this doesn't make sense, e.json() returns a string, this is effectively just json.dumps('foo') -> '"foo"'

This comment has been minimized.

Copy link
@hmvp

hmvp Sep 17, 2019

Author Contributor

Fixed

@samuelcolvin samuelcolvin requested review from tiangolo and removed request for tiangolo Sep 9, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Sep 17, 2019

Hi @hmvp would be great if you could work on fixing this. I'm trying to get a beta for version 1 out the door asap and I want to include this.

@hmvp

This comment has been minimized.

Copy link
Contributor Author

hmvp commented Sep 17, 2019

Sure, I will try to get a fixed MR out today

This fixes #620 and #793
@hmvp hmvp force-pushed the hmvp:fix_const_validations branch from 9260975 to e6d6389 Sep 17, 2019
@hmvp

This comment has been minimized.

Copy link
Contributor Author

hmvp commented Sep 17, 2019

Pushed an update

if getattr(self.type_, '__origin__', None):
# type_ has been refined eg. as the type of a List and sub_fields needs to be populated
self.sub_fields = [self._create_sub_type(self.type_, '_' + self.name)]
# type_ has been refined eg. as the type of a List and sub_fields needs to be populated

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 17, 2019

Owner

I've been working a lot on this part of the code in #803, I'm now sure this code will need more work.

But maybe it would be easier for me to fix it in #803.


m = Model()
assert m.a == [SubModel(b=1), SubModel(b=2), SubModel(b=3)]
assert m.b == [SubModel(b=4), SubModel(b=5), SubModel(b=6)]

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Sep 17, 2019

Owner

please create an issue about this.

@samuelcolvin samuelcolvin merged commit dd32a43 into samuelcolvin:master Sep 17, 2019
9 checks passed
9 checks passed
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190917.6 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.