Skip to content

fix: support json and yaml with separate decoding #509

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

Closed
wants to merge 3 commits into from

Conversation

tardyp
Copy link
Contributor

@tardyp tardyp commented Oct 11, 2021

Fix: #488

json is not exactly a subset of yaml, as yaml do not support tabs
there are maybe other subtleties, so we just use content-type to figure out how to parse

First commit actually fix the wrong part of the code 🤦 , but it might still be worth to have it behave similarly.

For a few lines of code, I am not sure it is really worth to factorize, so I didn't bother

Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, just one suggestion about JSON error handling.

@dbanty
Copy link
Collaborator

dbanty commented Oct 11, 2021

Thanks for adding this! I don't love the solution long-term because YAML is supposed to be a strict superset of JSON, it just seems that pyyaml only supports YAML spec 1.1, not 1.2 (I didn't know this before right now)!

The YAML 1.2 specification was published in 2009. Its primary focus was making YAML a strict superset of JSON. It also removed many of the problematic implicit typing recommendations. from https://yaml.org/spec/1.2.2/

Ideally, I'll switch to a different YAML parser that supports 1.2 (maybe ruamel). If that is easy enough I might do that instead of this PR.

In any event, getting some fix out is definitely a huge win, even if I end up changing it up long term. So thank you!!

@tardyp
Copy link
Contributor Author

tardyp commented Oct 11, 2021

@dbanty I have tested ruaml and it does not support tab as well.

I have updated the code and added a test for the error condition. good catch! sorry about this overlook. :-}

json is not technically a subset of yaml because yaml don't support tabs while json does

Signed-off-by: Pierre Tardy <[email protected]>
dbanty
dbanty previously approved these changes Oct 11, 2021
Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

It's a real shame that none of the YAML parsers for Python I could find properly support the 1.2 spec 😞. Thanks for putting this fix together, it's good to go once CI passes!

@tardyp tardyp changed the title support json and yaml with separate decoding fix: support json and yaml with separate decoding Oct 12, 2021
@tardyp
Copy link
Contributor Author

tardyp commented Oct 12, 2021

I would suggest to add a small "make check" makefile for new contributors, as the workflow approval requires you to approve, and auto-cancels itself at first failure.

@dbanty
Copy link
Collaborator

dbanty commented Oct 12, 2021

If you check out the CONTRIBUTING.md it tells you how to run tasks via Taskipy. In this case there's a task check from inside a poetry shell

json is not exactly a subset of yaml, as yaml do not support tabs
there are maybe other subtleties, so we just use content-type to figure out how to parse

Signed-off-by: Pierre Tardy <[email protected]>
@tardyp
Copy link
Contributor Author

tardyp commented Oct 12, 2021

I did look at contributing, but I missed the part about task check.
Just added a small documentation change that would have prevented me that overlook.

@dbanty
Copy link
Collaborator

dbanty commented Oct 12, 2021

Awesome, thanks! I definitely want it to be easier to contribute 😅.

@dbanty
Copy link
Collaborator

dbanty commented Oct 12, 2021

Blech, looks like the failing tests are due to some mocking that should probably be using tempfiles instead. I plan to do a sweep of open PRs to put together the next release this weekend, so don't fret over the failing tests if you don't have time, I'll fix them during that sprint.

@dbanty dbanty added this to the 0.10.6 milestone Oct 12, 2021
@tardyp
Copy link
Contributor Author

tardyp commented Oct 13, 2021

Interrestingly, thoses tests pass fine on my machine (python 3.9) this is why I did not see that.

@codecov
Copy link

codecov bot commented Oct 16, 2021

Codecov Report

Merging #509 (0a106eb) into main (a46c05a) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #509      +/-   ##
===========================================
- Coverage   100.00%   99.87%   -0.13%     
===========================================
  Files           48       48              
  Lines         1610     1628      +18     
===========================================
+ Hits          1610     1626      +16     
- Misses           0        2       +2     
Impacted Files Coverage Δ
openapi_python_client/__init__.py 99.52% <100.00%> (-0.48%) ⬇️
openapi_python_client/config.py 100.00% <100.00%> (ø)
..._client/schema/openapi_schema_pydantic/open_api.py 95.45% <0.00%> (-4.55%) ⬇️

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 a46c05a...0a106eb. Read the comment docs.

@dbanty
Copy link
Collaborator

dbanty commented Oct 16, 2021

Turns out those tests only fail on Python 3.7, I was able to reproduce by setting up an environment with that. Closing this in favor of #515 (which is this plus the 3.7 test fixes).

@dbanty dbanty closed this Oct 16, 2021
dbanty added a commit that referenced this pull request Oct 16, 2021
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.

Can't generate new project with OpenAPI JSON file
2 participants