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

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 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

@somada141 somada141 commented Jun 14, 2019

@samuelcolvin please have a look when you get a chance

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

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
Owner

@samuelcolvin samuelcolvin Jun 14, 2019

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

@somada141 somada141 Jun 14, 2019

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
Owner

@samuelcolvin samuelcolvin Jun 14, 2019

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

@somada141 somada141 Jun 14, 2019

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
Owner

@samuelcolvin samuelcolvin Jun 14, 2019

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

@somada141 somada141 Jun 14, 2019

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 Jun 14, 2019
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

@somada141 somada141 commented Jun 14, 2019

@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

@somada141 somada141 commented Jun 14, 2019

nvm realised you pushed a new version on it. updated

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

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 Jun 15, 2019
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

@somada141 somada141 commented Jun 15, 2019

@samuelcolvin updated

@samuelcolvin samuelcolvin merged commit 84820bd into samuelcolvin:master Jun 15, 2019
7 checks passed
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 15, 2019

merged, thanks so much.

Docs will be updated on the next release.

@somada141
Copy link
Contributor Author

@somada141 somada141 commented Jun 15, 2019

beautiful thanks a lot for the help @samuelcolvin

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