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.

Sign up for GitHub

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

Jump to bottom

Fix to schema generation for IPv{4,6}{Address,Interface,Network} #532

Merged
merged 5 commits into from
May 21, 2019
Merged

Fix to schema generation for IPv{4,6}{Address,Interface,Network} #532

merged 5 commits into from
May 21, 2019

Conversation

euri10
Copy link
Contributor

@euri10 euri10 commented May 20, 2019 •
edited
Loading

Change Summary

Added schema generation for all IPv{4,6}{Address,Interface,Network}, inspired by json-schema-org/json-schema-spec#498

The only thing modified that is of interest, and I'm not too sure about, is the order in field_class_to_schema_enum_disabled

Should have I kept the original order (Address > Interface > Network) then both test_ipv4interface_type and test_ipv6interface_type would fail with the following:

test_ipv4interface_type

E         Full diff:
E         - {'properties': {'ip_interface': {'format': 'ipv4address',
E         ?                                                  ^^^ --
E         + {'properties': {'ip_interface': {'format': 'ipv4interface',
E         ?                                                 ++++++ ^

With the order Network>Interface>Address all tests pass

Related issue number

complements json-schema-org/json-schema-spec#498

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 20, 2019

Codecov Report

Merging #532 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   json-schema-org/json-schema-spec#532   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2271   2271           
  Branches      447    447           
=====================================
  Hits         2271   2271

@samuelcolvin
Copy link
Member

I think you might also need to update the documentation.

@euri10
Copy link
Contributor Author

euri10 commented May 20, 2019

I think you might also need to update the documentation.

Done, hope I didn't miss another file to update ;)

@tiangolo tiangolo mentioned this pull request May 20, 2019
Open
tiangolo
tiangolo approved these changes May 20, 2019
View reviewed changes
Copy link
Collaborator

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Nice job @euri10 !

I just added a note to myself, in case they include an official format for IP Addresses in the JSON Schema spec, we should update it here.

But before that, this works well.

samuelcolvin
samuelcolvin reviewed May 20, 2019
View reviewed changes
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 LGTM.

HISTORY.rst Outdated
@@ -10,6 +10,7 @@ v0.26 (unreleased)
* fix return type hint for ``create_model``, #526 by @dmontagu
* **Breaking Change:** fix ``.dict(skip_keys=True)`` skipping values set via alias (this involves changing
``validate_model()`` to always returns ``Tuple[Dict[str, Any], Set[str], Optional[ValidationError]]``), #517 by @sommd
* fix to schema generation for ``IPv4Address``, ``IPv6Address``, ``IPv4Interface``, ``IPv6Interface``, ``IPv4Network``, ``IPv6Network`` by @euri10
Copy link
Member

Choose a reason for hiding this comment

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

can you add the PR number here like other changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tiangolo
tiangolo requested changes May 21, 2019
View reviewed changes
Copy link
Collaborator

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

I just realized there's an official "format" for IP v4 and v6 addresses. Can you please update them @euri10 ?

pydantic/schema.py Outdated Show resolved Hide resolved
docs/schema_mapping.py Show resolved Hide resolved
docs/schema_mapping.py Show resolved Hide resolved
@euri10
Copy link
Contributor Author

euri10 commented May 21, 2019

I just realized there's an official "format" for IP v4 and v6 addresses. Can you please update them @euri10 ?

Just did it, 🦅 eyes !

@tiangolo
Copy link
Collaborator

Great, thanks @euri10 !

LGTM

@samuelcolvin samuelcolvin merged commit c0298a8 into pydantic:master May 21, 2019
@samuelcolvin
Copy link
Member

awesome. Thank you both.

gangefors added a commit to gangefors/pydantic that referenced this pull request May 31, 2019
@gangefors
Merge remote-tracking branch 'upstream/master'
4af5c8d 
* 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)
  ...
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
@dmontagu
Don't recurse into metadata (pydantic#532)
e2a0bde 
* Don't recurse into metadata

* Add one more test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samuelcolvin samuelcolvin samuelcolvin left review comments

@tiangolo tiangolo tiangolo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@euri10 @samuelcolvin @tiangolo