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

Add parse_as_type function #934

Merged
merged 12 commits into from Nov 25, 2019
Merged

Conversation

@dmontagu
Copy link
Collaborator

dmontagu commented Oct 25, 2019

Change Summary

Adds just the parse_as_type function from #812, along with some documentation.

The documentation may need some work; @samuelcolvin if you give me some direction on what you'd like I'd be happy to modify it. I also think it may make sense to flesh it out a little more
and/or place it on a separate "tools" page once the dump_as_type functionality is implemented.

Related issue number

Addresses minor parts of #811, but to close that issue the dump_as_type functionality will be more important.

Given the lengthy back and forth on #812 I figured it might make more sense to just start fresh and break the pull request into smaller chunks.

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>.md file added describing change
    (see changes/README.md for details)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 25, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #934   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          19     20    +1     
  Lines        3293   3317   +24     
  Branches      651    653    +2     
=====================================
+ Hits         3293   3317   +24
Impacted Files Coverage Δ
pydantic/tools.py 100% <100%> (ø)

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 62bc930...315395b. Read the comment docs.

@aldanor

This comment has been minimized.

Copy link

aldanor commented Oct 27, 2019

This doesn't seem to work with dataclasses, is it supposed to?

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Oct 27, 2019

It should work if and only if you could put dataclasses as fields on a pydantic model.

I don't know whether @samuelcolvin would want to support that or not.

docs/usage/types.md Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
docs/usage/types.md Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
from pydantic.main import create_model

type_name = getattr(type_, '__name__', str(type_))
return create_model(f'ParsingModel[{type_name}] (for {source})', obj=(type_, ...))

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Nov 1, 2019

Owner

Not not sure about obj I think it could be confusing, I think we should use custom root types, e.g. __root__.

That might involve fixing #908 but I think that's a good idea anyway.

@dmontagu dmontagu mentioned this pull request Nov 2, 2019
4 of 4 tasks complete
@ahirner

This comment has been minimized.

Copy link
Contributor

ahirner commented Nov 11, 2019

Is this a partial solution for #481 and what is missing?

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 11, 2019

Is this a partial solution for #481 and what is missing?

This is orthogonal to the points raised in that issue, but I think it would be nice if it could support pydantic dataclasses too. I'm not sure how I'd make that happen though. I think if you added the functionality discussed in that issue, it would probably make it easier to have the parse_as_type function handle pydantic dataclasses.

Edit: Actually, it's not quite orthogonal, but it goes beyond what is discussed in that issue -- it supports parsing any type parseable by pydantic, not just models / dataclasses.

@ahirner

This comment has been minimized.

Copy link
Contributor

ahirner commented Nov 11, 2019

i like the tests, especially test_parse_as_type_preserves_subclasses and think it would be no problem to add functionality to achieve the same with dataclasses.
so correct me if I'm wrong: parse_as_type(obj, dataclass_or_model_type) == parse_obj(dataclass_or_model_type, obj) ?

edit: clarified input to mean types

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 11, 2019

so correct me if I'm wrong: parse_as_type(obj, dataclass_or_model_type) == parse_obj(dataclass_or_model_type, obj) ?

Yeah, I ordered arguments based on what seemed intuitive to me from the function name parse_as_type -> parse obj as type type_), but I am happy to reverse if preferable. The other order would be more consistent with typing.cast, so maybe that's better.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 11, 2019

Just waiting on #958 to replace obj with __root__.

pydantic/tools.py Outdated Show resolved Hide resolved
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 11, 2019

@ahirner Actually, to my surprise, it looks like this works out of the box with dataclasses. (I added a test.)

This will also handle the parse_file function.

I might hold off on implementing the to_json() function because we'll want to add dump_as_type from #811, and that may affect the best way to implement the to_json function. On the other hand, there should be a to_json function that doesn't depend on the type you are dumping, so maybe it's fine to add now.

pydantic/tools.py Outdated Show resolved Hide resolved
Copy link
Owner

samuelcolvin left a comment

otherwise looks good.

pydantic/tools.py Outdated Show resolved Hide resolved
pydantic/tools.py Outdated Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
@dmontagu dmontagu force-pushed the dmontagu:parse_as_type branch from 50b7fd0 to 76a41ae Nov 24, 2019
Copy link
Owner

samuelcolvin left a comment

otherwise LGTM.

pydantic/tools.py Show resolved Hide resolved
tests/test_tools.py Outdated Show resolved Hide resolved
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 25, 2019

I'm still not sure about parse_obj as a name, I understand why you chose it but:

  1. It's not really that similar to BaseModel.parse_obj()
  2. If you're new to pydantic it's less understandable than parse_as_typeor whatever

I guess I'd prefer parse_as_type() and parse_file_as_type(). (Or maybe better parse_object_as() and parse_file_as()). Thoughts?

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 25, 2019

I'm fine with that; it seemed similar to me since it is basically like the unbound version of the BaseModel.parse_obj classmethod (that doesn't need to have a BaseModel passed as the first argument). But I have no qualms with using different names.

I'd be in favor of parse_object_as and parse_file_as. I can change it to that if you confirm that's what you want. (If you want to think about it a little longer that's fine too.)

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Nov 25, 2019

Sorry to be indecisive. Let's go with parse_object_as and parse_file_as.

David Montague
@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Nov 25, 2019

@samuelcolvin I did the renames and added some docs; assuming you have no issues with the docs, I think this is just waiting now on changing obj to __root__, which requires #958.

(To be clear, I'm fine with this whether we use __root__ or obj though; I don't know if there is a performance implication to using a custom root model for this, but I'm guessing not?.)

@samuelcolvin samuelcolvin merged commit 6564bbb into samuelcolvin:master Nov 25, 2019
11 checks passed
11 checks passed
Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to 62bc930
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic #20191125.15 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
samuelcolvin.pydantic (Job Python38) Job Python38 succeeded
Details
MrMrRobat added a commit to MrMrRobat/pydantic that referenced this pull request Nov 28, 2019
* Add parse_as_type function

* Add changes

* Incorporate feedback

* Add naming tests

* Fix double quotes

* Fix docs example

* Reorder parameters; add dataclass and mapping tests

* Rename parse_as_type to parse_obj, and add parse_file

* Incorporate feedback

* Incorporate feedback

* use custom root types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.