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

Include all annotated fields in order #715

Merged
merged 5 commits into from Aug 12, 2019

Conversation

@dmontagu
Copy link
Collaborator

dmontagu commented Aug 5, 2019

Change Summary

Modifies field ordering so that all annotated fields occur in the order included.

Note: non-annotated fields still come last; this is probably unavoidable.

This is a subtly breaking change, so, if desired, should probably be part of 1.0.

Related issue number

Related to #674 #483 #203 #716

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

This comment has been minimized.

Copy link

codecov bot commented Aug 5, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #715   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2710   2807   +97     
  Branches      532    578   +46     
=====================================
+ Hits         2710   2807   +97
@samuelcolvin samuelcolvin added this to the Version 1 milestone Aug 6, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Aug 6, 2019

please can you rebase and move the history change to changes/ as per #719.

@dmontagu dmontagu force-pushed the dmontagu:field-ordering branch from 0dc8323 to 21dd6a5 Aug 7, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Aug 8, 2019

as per #716 I guess we should implement Config.require_annotations = True

Copy link
Owner

samuelcolvin left a comment

otherwise looks good, plus Config.require_annotations = True

name=var_name,
value=value,
annotation=annotations.get(var_name),
class_validators=vg.get_validators(var_name),
config=config,
)
new_fields_ordering = [k for k in annotations if k in new_fields] + [

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner
Suggested change
new_fields_ordering = [k for k in annotations if k in new_fields] + [
ordered_new_fields = [k for k in annotations if k in new_fields] + [
new_fields_ordering = [k for k in annotations if k in new_fields] + [
k for k in new_fields if k not in annotations
]
for k in new_fields_ordering:

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner
Suggested change
for k in new_fields_ordering:
fields.update(ordered_new_fields)

I think.

This comment has been minimized.

Copy link
@dmontagu

dmontagu Aug 8, 2019

Author Collaborator

new_fields_ordering only includes the keys, not the values; your modification above didn't change that, as far as I can tell. So ordered_new_fields is a List[str], not a Dict[str, Field].

I'm happy to rename, but I couldn't come up with a way to get ordered_new_fields to be a dict without making things ugly elsewhere. Let me know if you want me to try harder.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner

Humm, that's true sorry.

Rather than describe it all, i'll commit to your PR and you can change or remove it if it doesn't make sense.

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 8, 2019

Owner

take a look at d3b10e5, I think that's better than building the order wrong then rebuilding it right, but happy to change if I'm missing something.

David Montague added 2 commits Aug 5, 2019
@dmontagu dmontagu force-pushed the dmontagu:field-ordering branch from 21dd6a5 to 62b543b Aug 8, 2019
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Aug 8, 2019

@dmontagu kind of unrelated, but nowhere better to discuss:

Could I have your email address? If you're not happy sharing it publicly, feel free to email s@muelcolvin.com.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Aug 9, 2019

@dmontagu kind of unrelated, but nowhere better to discuss:

Could I have your email address? If you're not happy sharing it publicly, feel free to email s@muelcolvin.com.

Sent you an email, let me know if you didn't get it.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Aug 12, 2019

@dmontagu are you happy with this? If so I guess it can be merged.

@dmontagu

This comment has been minimized.

Copy link
Collaborator Author

dmontagu commented Aug 12, 2019

@samuelcolvin I'm good with it, feel free to merge once the checks re-pass.

@samuelcolvin samuelcolvin merged commit 321cde0 into samuelcolvin:master Aug 12, 2019
10 checks passed
10 checks passed
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to e6c44ee
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190812.5 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.