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

feat(field): add dump_alias and load_alias options #1695

Closed
wants to merge 2 commits into from

Conversation

PrettyWood
Copy link
Member

@PrettyWood PrettyWood commented Jul 7, 2020

Change Summary

  • add dump_alias option
  • add load_alias option
  • generate schema for both serialization and deserialization
  • add documentation

Related issue number

closes #624

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 Jul 7, 2020

Codecov Report

Merging #1695 (98e91c7) into master (7b7e705) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 98e91c7 differs from pull request most recent head 2063491. Consider uploading reports for the commit 2063491 to get more accurate results

@@             Coverage Diff             @@
##            master     #1695     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           25        21      -4     
  Lines         5109      3921   -1188     
  Branches      1050       785    -265     
===========================================
- Hits          5109      3921   -1188     
Impacted Files Coverage Δ
pydantic/fields.py 100.00% <100.00%> (ø)
pydantic/json.py 100.00% <0.00%> (ø)
pydantic/mypy.py 100.00% <0.00%> (ø)
pydantic/tools.py 100.00% <0.00%> (ø)
pydantic/types.py 100.00% <0.00%> (ø)
pydantic/utils.py 100.00% <0.00%> (ø)
pydantic/errors.py 100.00% <0.00%> (ø)
pydantic/schema.py 100.00% <0.00%> (ø)
pydantic/typing.py 100.00% <0.00%> (ø)
... and 11 more

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 b718e8e...2063491. Read the comment docs.

@PrettyWood PrettyWood marked this pull request as ready for review July 23, 2020 16:22
@PrettyWood PrettyWood changed the title feat(field): add dump_alias and load_alias options [WIP] feat(field): add dump_alias and load_alias options Aug 9, 2020
@PrettyWood PrettyWood changed the title [WIP] feat(field): add dump_alias and load_alias options [wip] feat(field): add dump_alias and load_alias options Sep 16, 2020
@PrettyWood PrettyWood marked this pull request as draft September 16, 2020 12:59
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.

This is looking good.

Questions:

  • are we satisfied it'll work correctly with inheritance? The inheritance landscape with aliases is already mind numbingly complicated, will this just slot in or confuse matters more?
  • How do we deal with skipping/excluding from dict() etc.? I guess the simplest solution might be an object Exclude which can only be used on dump_alias.

@PrettyWood
Copy link
Member Author

PrettyWood commented Oct 10, 2020

@samuelcolvin This is the whole question and honestly I need to think it over. This is why it's just a draft. I feel like we still need some feedback on the desired API. The best would be to have a bunch of tests with nested models and load/dump + exclude fields to have the expected behaviour for each one and see if there are no edge cases or something

@daviskirk
Copy link
Contributor

daviskirk commented Jan 1, 2021

Just saw this while working on #660 ... not sure if I'm out of lying trying to create a pull requests for that or if the discussion here has changed the view on it.
Especially the idea of having an "Exclude" object on the dump alias is a bit different compared to #660 (comment).

That said, there are a few arguments for separating the two:

  1. having separation of concerns: the "exclude" functionality with an explicit "exclude" option is probably needed anyway because we probably want to support the "advanced" exclude syntax: t.dict(exclude={'user': {'username', 'password'}, 'value': ...}). Having another way of excluding a field using the dump_alias might be confusing as there would now be two ways of doing this.

  2. We might want to toggle exclusion separately from the dump alias, assuming we want to allow a case like this: Let's say we have a model

class TestModel(BaseModel):
    a: int = Field(0, dump_alias="a_dump", exclude=...)
    b: str = "foo"
    c: str = "bar"

would we want to allow the fields to be "included" by the .dict override??

TestModel(a=3).dict(include={"a"})  # {"a_dump": 3}

There are cases I know where this might be the desired behavior.

There are also arguments against the second point: I can just as well think of cases where this might be confusing because the author of the class wants the field to be hidden permanently and not being randomly resurrected by a overriding using dict or json. Especially confusing would might be a case like this:

TestModel(a=3).dict()  # {"b": "foo", "c": "bar"}
# user decides they don't want to see "b"
TestModel(a=3).dict(exclude={"b"})  # {"a_dump": 3, "c": "bar"} ... what?? why did a_dump show up, it wasn't there before.  

@samuelcolvin @PrettyWood Were there any thoughts on "dump_alias" vs. "exclude" in class config / field or am I just confusing a bunch of issues here?

@daviskirk daviskirk mentioned this pull request Jan 3, 2021
4 tasks
@samuelcolvin
Copy link
Member

What if we didn't allow excluding via dump_alias and load_alias - just allowed the alias to be changed?

That way we get clear separation of concerns and a simple interface.

@PrettyWood
Copy link
Member Author

What if we didn't allow excluding via dump_alias and load_alias - just allowed the alias to be changed?

That way we get clear separation of concerns and a simple interface.

I'm really in favor of this. I'll be back on this PR in the next few days. Thanks a lot @daviskirk for the suggestion. Sorry I missed your message before

@daviskirk
Copy link
Contributor

daviskirk commented Feb 19, 2021

@PrettyWood

I'm really in favor of this. I'll be back on this PR in the next few days. Thanks a lot @daviskirk for the suggestion. Sorry I missed your message before

#2231 now has a working candidate (see: #2231 (comment) in particular, in case that influences anything here)

@PrettyWood PrettyWood changed the title [wip] feat(field): add dump_alias and load_alias options feat(field): add dump_alias and load_alias options May 17, 2021
@PrettyWood
Copy link
Member Author

PrettyWood commented May 17, 2021

@samuelcolvin I went back on this.
I added load_alias and dump_alias and also changed schema generation to decide whether serialization ou deserialization schema should be generated.
I'll first wait for your input before writing documentation.

PS: fastapi tests fail because they use ModelField.alias but I reckon code can probably be updated on fastapi side.

@tiangolo I would like your input on this matter if possible. I reckon differentiating serialization and deseralization will be useful for fastapi and this is a start

@PrettyWood PrettyWood marked this pull request as ready for review May 17, 2021 23:17
@samuelcolvin
Copy link
Member

Closing this.

Although we'll definitely support load_alias and dump_alias in V2, I doubt we'll use any of this logic.

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.

load_alias and dump_alias
3 participants