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

feat: Adds support for parameter components and parameter references. #615

Conversation

jsanchez7SC
Copy link

@jsanchez7SC jsanchez7SC commented May 19, 2022

This PR partially resolves #288

@jsanchez7SC
Copy link
Author

@dbanty friendly ping

This PR should add a couple new regression tests as well but wanted to get your feedback first.

@jsanchez7SC
Copy link
Author

ping again @dbanty

@dbanty
Copy link
Collaborator

dbanty commented Jun 3, 2022

Sorry for the delays @jsanchez7SC, I don't have very much time to work on this project right now 😬. I've skimmed your PR (though not reviewed it in depth) and if I understand it correctly then it looks pretty good. You're collecting all the references to parameters and processing them so they can be consumed later by any referrers, right? Basically, the same way we handle other sorts of references (schemas) already.

I've approved CI so that you can follow up on any issues that pop up there. Hopefully, I'll get the time to do a more in-depth review soon.

@jsanchez7SC
Copy link
Author

I've been OOO for the last couple of weeks. Will get back to the PR this week.

@jsanchez7SC
Copy link
Author

jsanchez7SC commented Jul 11, 2022

Mind allowing the workflows to run @dbanty? Is there any way to allowlist this PR so I can finish developing and trigger CI by myself with shorter roundtrips?

Thanks :-)

@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #615 (59f17fc) into main (d512d8b) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 59f17fc differs from pull request most recent head c04402b. Consider uploading reports for the commit c04402b to get more accurate results

@@            Coverage Diff            @@
##              main      #615   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           49        49           
  Lines         1713      1783   +70     
=========================================
+ Hits          1713      1783   +70     
Impacted Files Coverage Δ
openapi_python_client/schema/__init__.py 100.00% <ø> (ø)
openapi_python_client/parser/errors.py 100.00% <100.00%> (ø)
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
openapi_python_client/parser/properties/schemas.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dbanty
Copy link
Collaborator

dbanty commented Jul 11, 2022

Mind allowing the workflows to run @dbanty?

Done!

Is there any way to allowlist this PR so I can finish developing and trigger CI by myself with shorter roundtrips?

I don't know of a way to do that 😕. You should be able to run the checks in your fork though, since there's nothing special about the tests variable-wise. I think the only ones you wouldn't be able to run in your fork are the release-related ones.

@jsanchez7SC
Copy link
Author

Mind allowing the workflows to run @dbanty?

Done!

Is there any way to allowlist this PR so I can finish developing and trigger CI by myself with shorter roundtrips?

I don't know of a way to do that 😕. You should be able to run the checks in your fork though, since there's nothing special about the tests variable-wise. I think the only ones you wouldn't be able to run in your fork are the release-related ones.

Got it. So purely task check yeah? Will continue doing so.
Ok, I'm going to fix the code coverage but in the meantime we should be good for a full review if you plan on doing one.

Adds missing tests for a number of parameter-related methods.
@jsanchez7SC
Copy link
Author

Code coverage should be fixed now. Ready for a review

@jsanchez7SC
Copy link
Author

Friendly ping for a review when you have time @dbanty

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.

Looks good! The only changes are the couple of things I pointed out in the review, and could you add an e2e test? All you have to do is add some schemas you expect to work into end_to_end_tests/openapi.json (just nab a real-life use-case if you have one), then run poetry run task re to regenerate the "golden record" code.

Then we just check that the generated code looks right for the new endpoints & references. It's sort of a last-minute sanity check that the new feature does the thing we expect it to do when run on valid schema.

Thanks so much for implementing this and sorry for all the delays on reviews!

openapi_python_client/parser/openapi.py Outdated Show resolved Hide resolved
openapi_python_client/parser/properties/schemas.py Outdated Show resolved Hide resolved
tests/test_parser/test_openapi.py Show resolved Hide resolved
Jordi Sanchez added 2 commits August 10, 2022 21:49
Removes a nonexisting parameter from update_parameters_with_data's docstring.
Adds an end-to-end test for parameters passed by reference.
@dbanty
Copy link
Collaborator

dbanty commented Aug 13, 2022

Thanks for all your work on this @jsanchez7SC! I'm just going to update a few tests and then merge via #653.

@dbanty dbanty closed this Aug 13, 2022
@dbanty dbanty added this to the 0.11.5 milestone Aug 13, 2022
dbanty added a commit that referenced this pull request Aug 13, 2022
…. Thanks @jsanchez7SC!

Closes #288.

Co-authored-by: Jordi Sanchez <jsanchez7@snap.com>
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.

Support non-schema components and references (e.g. parameters)
2 participants