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(encoder): serialize NameEmail to str #2479

Merged

Conversation

alecgerona
Copy link
Contributor

@alecgerona alecgerona commented Mar 5, 2021

Change Summary

Make NameEmail json serializable.

Related issue number

closes #2341

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

codecov bot commented Mar 5, 2021

Codecov Report

Merging #2479 (ab6f11c) into master (619ff26) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2479   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines         5109      5110    +1     
  Branches      1050      1050           
=========================================
+ Hits          5109      5110    +1     
Impacted Files Coverage Δ
pydantic/json.py 100.00% <100.00%> (ø)

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 619ff26...ab6f11c. Read the comment docs.

@alecgerona alecgerona marked this pull request as draft March 5, 2021 11:01
@alecgerona alecgerona marked this pull request as ready for review March 5, 2021 11:08
Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

You need to explain your change in changes/2341-alecgerona.md e.g.
allow serialization of NameEmail

@alecgerona
Copy link
Contributor Author

@PrettyWood weird my only change was that I added one line to the md file as you said but CI / test fastAPI suddenly didn't work. Is this just a case of retry or am I missing something?

Copy link
Member

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

LGTM!

@hinnerk
Copy link

hinnerk commented Apr 29, 2021

This should be fixed upstream, see #2630. Can you re-run the test and then possibly merge?

I'm eagerly awaiting this. ;)

Edit: Never mind, fastAPI hasn't released 0.64.0 yet. It hasn't released anything this year, in fact. Does that mean that pydantic is stuck with this issue?

@samuelcolvin samuelcolvin merged commit 9f654a1 into pydantic:master May 9, 2021
@samuelcolvin
Copy link
Member

👍 thanks so much.

@alecgerona
Copy link
Contributor Author

Thanks @samuelcolvin! Was trying to find the time to rerun the tests 😂. Glad you merged it still.

@hinnerk
Copy link

hinnerk commented May 10, 2021

Wonderful, thank you!

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

Successfully merging this pull request may close these issues.

Object of type 'NameEmail' is not JSON serializable
4 participants