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 __fields_set__ not using alias field names (#517) #518

Merged
merged 11 commits into from
May 18, 2019
Merged

Fix __fields_set__ not using alias field names (#517) #518

merged 11 commits into from
May 18, 2019

Conversation

sommd
Copy link
Contributor

@sommd sommd commented May 12, 2019

fix #517

Change Summary

Basically, instead of just setting __fields_set__ from input_data.keys() (which could contain aliases instead of the field names), it checks all the fields and adds the field's name if it or the alias is in input_data. Not sure if this is the best way to do this.

Related issue number

#517

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 May 12, 2019

Codecov Report

Merging #518 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #518   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2272   2271    -1     
  Branches      448    447    -1     
=====================================
- Hits         2272   2271    -1

pydantic/main.py Outdated Show resolved Hide resolved
pydantic/main.py Outdated
else:
fields_set = data.keys() & self.__values__.keys() # type: ignore
object.__setattr__(self, '__fields_set__', fields_set)
object.__setattr__(self, '__fields_set__', self._process_fields(data))
Copy link
Member

Choose a reason for hiding this comment

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

humm, I'm not sure this is the easiest or most efficient way of doing things. It might be easier to build __fields_set__ in validate_model.

I think you can change like ~610 to

else:
    names_used.add(field.name if using_name else field.alias)

Then add names_used along with the other results.

Copy link
Contributor Author

@sommd sommd May 14, 2019

Choose a reason for hiding this comment

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

validate_model currently doesn't mutate the model object at all, so if someone is using it then it may break unexpectedly. There's also no way to return the names_used. If you think it's OK to call object.__setattr__(model, '__fields_set__', names_used) from validate_model then I can change it to do that.

Copy link
Member

Choose a reason for hiding this comment

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

I would change the signature of validate_model to return a tuple including fields_set.

No one should be using validate_model directly so this should be fine.

Copy link
Member

Choose a reason for hiding this comment

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

It was a mistake to have to different return types for validate_model, best to change it to always return Tuple['DictStrAny', Set[str], Optional[ValidationError]]

Copy link
Member

Choose a reason for hiding this comment

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

If that's too much work for this PR, I could implement a PR to make the return type always Tuple['DictStrAny', Optional[ValidationError]], then you could rebase off that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Managed to do it. Might need a bit more testing but it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also if no one should be using validate_model model it should probably be _validate_model or at least not exported in __init__.

tests/test_main.py Outdated Show resolved Hide resolved
tiangolo added a commit to tiangolo/pydantic that referenced this pull request May 14, 2019
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 looking good.

pydantic/main.py Show resolved Hide resolved
HISTORY.rst Outdated Show resolved Hide resolved
tests/test_main.py Outdated Show resolved Hide resolved
pydantic/main.py Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin merged commit cb2abb1 into pydantic:master May 18, 2019
@samuelcolvin
Copy link
Member

awesome, thank you very much.

tiangolo added a commit to tiangolo/pydantic that referenced this pull request May 18, 2019
gangefors added a commit to gangefors/pydantic that referenced this pull request May 31, 2019
* upstream/master: (138 commits)
  add 'none-any.whl' to pypi upload (pydantic#564)
  uprev
  update benchmarks (pydantic#563)
  cython (pydantic#548)
  Fix issue with unspecified generic type (pydantic#554)
  Run dataclass' original __post_init__ before validation (pydantic#560)
  try to stop annoying warnings in azure pipeline (pydantic#549)
  azure pipeline failOnStderr: false
  Azure Pipelines - tests for windows (pydantic#538)
  Fix JSON Schema for list, tuple, and set, improving interoperability (pydantic#540)
  uprev.
  Colors (pydantic#516)
  Fix to schema generation for IPv{4,6}{Address,Interface,Network} (pydantic#532)
  Fix __fields_set__ not using alias field names (pydantic#517) (pydantic#518)
  Change return type hint for create_model (pydantic#526)
  Tuple ellipsis (pydantic#512)
  Fix to schema generation for IPvAny{Address,Interface,Network} (pydantic#498) (pydantic#510)
  uprev
  Scheduled monthly dependency update for May (pydantic#499)
  Implement const keyword in Schema. (pydantic#469)
  ...
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.

.dict(skip_defaults=True) skips fields populated by alias
2 participants