Skip to content

feat: add support for lists of basic python types #165

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
Aug 28, 2020

Conversation

Maistho
Copy link
Contributor

@Maistho Maistho commented Aug 19, 2020

Adds support for lists of strings/numbers etc

Let me know if there should be some test changes or such. I haven't contributed to this repository before, so I'd appreciate some pointers if so. :)

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #165 into main will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #165   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           41        41           
  Lines         1264      1276   +12     
=========================================
+ Hits          1264      1276   +12     
Impacted Files Coverage Δ
openapi_python_client/parser/responses.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 11adc3a...3cca614. Read the comment docs.

@dbanty
Copy link
Collaborator

dbanty commented Aug 19, 2020

Awesome, thanks for the contribution 😃! We do try to keep 100% unit test coverage as well as add end to end tests where feasible. General info should be in the CONTRIBUTING.md (though I just realized the layout is a bit misleading).

In this case since your changes are in openapi_python_client/parser/responses.py the unit tests should be in tests/test_parser/test_responses.py. You'll want to cover the new branch in response_from_data with a new function in TestResponseFromData. You'll notice the tests all start with test_<name_of_function> which is repetitive and annoying but it helps PyCharm locate tests 😁.

You'll also want to cover the new ListBasicResponse class with a TestListBasicResponse which you can probably do by just copying TestBasicResponse and tweaking as needed.

For the end to end tests, right now it's all based on a FastAPI app. The easiest thing to do would be to create a new endpoint which returns a basic list response, then run task oge to regenerate the "golden-master" which is a generated client. I realize basing this on FastAPI is probably not the most accessible thing, so if you want to leave that out I can add it in for you.

Finally, stick a summary of your addition in a bullet in CHANGELOG.md, feel free to include a thanks @Maistho in there to credit yourself 😄.

@dbanty dbanty added this to the 0.5.4 milestone Aug 19, 2020
@dbanty dbanty self-requested a review August 19, 2020 18:05
@Maistho
Copy link
Contributor Author

Maistho commented Aug 23, 2020

Hi, thanks for the detailed response. I hadn't the time last week but I'll make sure to write the tests and documentation changes required next week.

FastAPI is fine, that's what we use for our projects anyway.

Added support for lists of strings, floats, integers and booleans.
@Maistho
Copy link
Contributor Author

Maistho commented Aug 27, 2020

I've implemented the unit and e2e tests, and added a note in the CHANGELOG.md now.

I also rebased on the latest changes on the main branch.

I guess there shouldn't be any other changes, such as documentation, required?

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.

Awesome, thanks for all your work on this @Maistho!

response = httpx.get(url=url, headers=headers,)

if response.status_code == 200:
return [str(item) for item in cast(List[str], response.json())]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably don't need the cast here since we're converting to the correct type with str. I suppose it doesn't hurt though, so we can leave it in.

@dbanty dbanty merged commit 36e6755 into openapi-generators:main Aug 28, 2020
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