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

Add additional types from the specification. #526

Closed
wants to merge 2 commits into from

Conversation

ngnpope
Copy link
Contributor

@ngnpope ngnpope commented Sep 17, 2021

The first commit adds some additional type mappings for more Python types available in the standard library.

The second commit adds support for missing formats that are defined in the specification:

I guess many of these may be too obscure. If you don't want them, we could perhaps just add regex as that seems most useful.

@ngnpope ngnpope changed the title Additional types Add additional types from the specification. Sep 17, 2021
@tfranzel
Copy link
Owner

hey @ngnpope, actually love this PR! thing is i deliberately only added types that are explicitly mentioned in 3.0.3.

3.0.3 is not based on JSON schema directly. However, this changed with 3.1. So in theory those types are not officially recognized by 3.0.3 and thus may likely be ignored by upstream tooling. My problem is that user may not know these minutiae and could expect the provided types to work.

On the other hand these types will get support at some point in time and tooling might already include them.

@ngnpope
Copy link
Contributor Author

ngnpope commented Sep 19, 2021

When comparing to the data types section of the v3.0.3 specification, it seems that there are already additional formats supported in drf-spectacular that are not in mentioned in the table - notably uuid, uri, ipv4, ipv6, hostname, time, duration, and email. The specification does say, however:

Formats such as "email", "uuid", and so on, MAY be used even though undefined by this specification. ... Tools that do not recognize a specific format MAY default back to the type alone, as if the format is not specified.

So, I guess from that perspective, it shouldn't be a problem defining these and it is on step toward OpenAPI 3.1 that is backward compatible. Maybe we just need to expand the documentation to say which ones are defined under which specifications?

Either way, I think the first commit should land for mapping from Python types. (I also notice that I accidentally included that .TIME and .DURATION are mapped from datetime.time and datetime.timedelta in the enum documentation update from the other day which is not yet true... Sorry for that oversight. 😬)

@codecov
Copy link

codecov bot commented Sep 19, 2021

Codecov Report

Merging #526 (e9a57bc) into master (e9a435c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #526   +/-   ##
=======================================
  Coverage   98.62%   98.62%           
=======================================
  Files          58       58           
  Lines        6028     6038   +10     
=======================================
+ Hits         5945     5955   +10     
  Misses         83       83           
Impacted Files Coverage Δ
drf_spectacular/utils.py 98.90% <ø> (ø)
drf_spectacular/types.py 100.00% <100.00%> (ø)

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 e9a435c...e9a57bc. Read the comment docs.

@tfranzel
Copy link
Owner

that's a fair point. i agree the benefits outweigh the potential problems

i removed Pattern from the mapping because mypy complained and it is not available in 3.6.

i merged the PR without noticing it was a ff. github does not understand this apparently. your commits are not on master so i'll close this.

@tfranzel tfranzel closed this Sep 19, 2021
@tfranzel
Copy link
Owner

i meant: your commits are now on master. worst possible typo for that confusing PR state. sry.

@ngnpope ngnpope deleted the additional-types branch September 19, 2021 21:25
@ngnpope
Copy link
Contributor Author

ngnpope commented Sep 19, 2021

i meant: your commits are now on master. worst possible typo for that confusing PR state. sry.

😂 Thank you!

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

2 participants