Skip to content

allow unknown string format properties #65

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

Merged
merged 1 commit into from
Jun 13, 2020

Conversation

acgray
Copy link

@acgray acgray commented Jun 8, 2020

Addresses #64

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #65   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          671       669    -2     
=========================================
- Hits           671       669    -2     
Impacted Files Coverage Δ
openapi_python_client/openapi_parser/properties.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 05b3784...3cbc3a3. Read the comment docs.

@dbanty
Copy link
Collaborator

dbanty commented Jun 8, 2020

Thanks @acgray this looks good. The only concern I have is that support for the "byte" format has not been added yet, and I would expect that type to use the Python bytes type instead of str. Could you add back in the parse error specifically for that format? You could also add support for the format if you're feeling up to it.

@acgray
Copy link
Author

acgray commented Jun 8, 2020

Sure, although given the content-type is currently assumed to be application/json, the case of a field containing raw bytes inside a JSON object seems to me to be relatively unlikely. Did you have a use-case in mind that I'm missing?

@dbanty
Copy link
Collaborator

dbanty commented Jun 8, 2020

Sure, we work with IoT which pushes some packed binary data up to save space. Most of these don't use an OpenAPI endpoint right now but we're moving everything in that direction. Some visible metadata can go in JSON keys while the main fat payload can be a big binary blob.

We do also support "application/x-www-form-urlencoded" and "multipart/form-data" in requests and "text/html" in responses. Though now that you mention it, I wonder if httpx is always picking the right content-type based on payload >.>. We don't explicitly set it today.

@dbanty
Copy link
Collaborator

dbanty commented Jun 13, 2020

After some more right on this, you're right @acgray. Embedding binary data within JSON is rare enough that we probably don't need to add support. With the generic string feature, that data should come through as a base64 encoded string, at which point the developers working with binary data presumably know what they want to do with it (which might not be the bytes type anyway).

I'm going to merge this one in and if someone complains about manually decoding data we can revisit decoding it in here.

@dbanty dbanty closed this Jun 13, 2020
@dbanty dbanty reopened this Jun 13, 2020
@dbanty dbanty merged commit a4f57ef into openapi-generators:master Jun 13, 2020
@acgray
Copy link
Author

acgray commented Jun 14, 2020 via email

@dbanty
Copy link
Collaborator

dbanty commented Jun 14, 2020

@acgray not a problem at all. Thanks for all your contributions!

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.

2 participants