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

Clarification around usage of Union #594

Merged
merged 16 commits into from Jun 15, 2019

Conversation

somada141
Copy link
Contributor

@somada141 somada141 commented Jun 14, 2019

Change Summary

Adds examples of the incorrect and correct usage of Union when defining type annotations with multiple types and updated documentation. This is discussed further under #514

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>

@codecov
Copy link

codecov bot commented Jun 14, 2019

Codecov Report

Merging #594 into master will decrease coverage by 2.71%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #594      +/-   ##
==========================================
- Coverage     100%   97.28%   -2.72%     
==========================================
  Files          15       15              
  Lines        2542     2542              
  Branches      504      504              
==========================================
- Hits         2542     2473      -69     
- Misses          0       58      +58     
- Partials        0       11      +11

@somada141
Copy link
Contributor Author

@samuelcolvin please have a look when you get a chance

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Can you also add an entry to HISTORY.rst, I know this isn't a code change but it's still useful in the changelog and it gets your name into contributors.

docs/index.rst Outdated

(This script is complete, it should run "as is")

However, as can be seen above, *pydantic* will attempt to 'match' any of the models (classes) defined under ``Union`` and will use the first one that matches. In the above example the ``id`` of ``user_03`` was defined as a ``uuid.UUID`` class (which is defined under the attribute's ``Union`` annotation) but as the ``uuid.UUID`` can be marshalled into an ``int`` it chose to match against the ``int`` type and disregarded the other types.
Copy link
Member

Choose a reason for hiding this comment

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

please hard wrap this text.

Also models (classes) can changed to types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/index.rst Outdated

However, as can be seen above, *pydantic* will attempt to 'match' any of the models (classes) defined under ``Union`` and will use the first one that matches. In the above example the ``id`` of ``user_03`` was defined as a ``uuid.UUID`` class (which is defined under the attribute's ``Union`` annotation) but as the ``uuid.UUID`` can be marshalled into an ``int`` it chose to match against the ``int`` type and disregarded the other types.

As such, it is recommended that when defining ``Union`` annotations that the most specific model (class) is defined first and followed by less specific models (classes). In the above example, the ``UUID`` class should precede the ``int`` and ``str`` classes to preclude the unexpected representation as such:
Copy link
Member

Choose a reason for hiding this comment

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

same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/index.rst Outdated
@@ -949,6 +949,24 @@ Benchmarks were run with python 3.7.2 and the following package versions:
* **django-restful-framework** ``v3.9.4``


Usage of ``Union`` in Annotations and Type Order
Copy link
Member

Choose a reason for hiding this comment

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

can you move this above benchmarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

docs/examples/union_type_correct.py Outdated Show resolved Hide resolved
docs/examples/union_type_incorrect.py Outdated Show resolved Hide resolved
docs/examples/union_type_incorrect.py Outdated Show resolved Hide resolved
docs/examples/union_type_incorrect.py Outdated Show resolved Hide resolved
somada141 and others added 7 commits June 15, 2019 08:06
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@somada141
Copy link
Contributor Author

@samuelcolvin I updated HISTORY.rst but didn't set a version for the release, does the way I added to it make sense?

@somada141
Copy link
Contributor Author

nvm realised you pushed a new version on it. updated

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise fine.

HISTORY.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
somada141 and others added 4 commits June 15, 2019 21:34
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
Co-Authored-By: Samuel Colvin <samcolvin@gmail.com>
@somada141
Copy link
Contributor Author

@samuelcolvin updated

@samuelcolvin samuelcolvin merged commit 84820bd into pydantic:master Jun 15, 2019
@samuelcolvin
Copy link
Member

merged, thanks so much.

Docs will be updated on the next release.

@somada141
Copy link
Contributor Author

beautiful thanks a lot for the help @samuelcolvin

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
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

2 participants