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

Enumbug #204

Closed
wants to merge 10 commits into from
Closed

Enumbug #204

wants to merge 10 commits into from

Conversation

euri10
Copy link
Contributor

@euri10 euri10 commented May 7, 2019

Attempt at fixing #196

I have to admit I got no idea if the reported behavior by @ebreton is effectively a bug though, according to Pydantic docs the use_enum_values is quote:

whether to populate models with the value property of enums, rather than the raw enum - useful if you want to serialise model.dict() later (default: False)

the default is effectively False so in the edfault case, it uses the raw value of the Enum, not the key of it, hence why I'm not sure if it's the right intent.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #204 into master will decrease coverage by 0.02%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage     100%   99.97%   -0.03%     
==========================================
  Files         170      170              
  Lines        4122     4130       +8     
==========================================
+ Hits         4122     4129       +7     
- Misses          0        1       +1
Impacted Files Coverage Δ
tests/test_jsonable_encoder.py 100% <100%> (ø) ⬆️
fastapi/encoders.py 97.87% <75%> (-2.13%) ⬇️

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 d03678d...8a1ceaa. Read the comment docs.

@tiangolo
Copy link
Owner

Thanks @euri10 , I'll check it soon.

@tiangolo
Copy link
Owner

Thanks @euri10 , I'm delaying this until pydantic/pydantic#520 is resolved.

It will change how responses are encoded from Pydantic models in a much cleaner way. And it will solve several issues at the same time (most probably including this).

@tiangolo
Copy link
Owner

tiangolo commented Aug 4, 2019

I think this should be solved in recent versions, so I'll close this now, but feel free to add more comments/issues/PRs 😉 🍰

@tiangolo tiangolo closed this Aug 4, 2019
@ebreton
Copy link

ebreton commented Sep 7, 2019

Hi @tiangolo , I have refactored my projects with the orm_mode and the latest updates from both pydantic and fastapi. Enum are still encoded with their values, whatever the Config is...

I think this PR is still relevant

@ebreton
Copy link

ebreton commented Sep 9, 2019

Changing my mind. I think I did not understand correctly pydantic documentation on use_enum_values:

whether to populate models with the value property of enums, rather than the raw enum - useful if you want to serialise model.dict() later (default: False)

I understand now that it is just a matter of passing either the Enum object, or the values (i.e. strings). I though I could choose between the name and the value, but that is not the case.

My issue comes from full-stack-fastapi-postgresql, I will update tiangolo/full-stack-fastapi-template#23 with my findings

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

3 participants