Skip to content

fix: Prevent duplicate return types in generated api functions #365

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 6 commits into from
Apr 5, 2021

Conversation

dbanty
Copy link
Collaborator

@dbanty dbanty commented Mar 27, 2021

No description provided.

@codecov
Copy link

codecov bot commented Mar 27, 2021

Codecov Report

Merging #365 (f152619) into main (a4e3e6a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #365   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1483      1490    +7     
=========================================
+ Hits          1483      1490    +7     
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.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 a4e3e6a...f152619. Read the comment docs.

@dbanty dbanty requested a review from emann March 30, 2021 14:04
emann
emann previously approved these changes Apr 5, 2021
Copy link
Collaborator

@emann emann left a comment

Choose a reason for hiding this comment

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

A question and a suggestion but LGTM as far as implementation goes.

@@ -265,6 +265,15 @@ def from_data(

return result, schemas

def response_type(self) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason this isn't a @property?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generally bad practice to make something a property if it does a bunch of work. The method call () indicates to the caller that something is happening, so if you need it twice you should assign it to a variable rather than calling it twice.

@@ -114,6 +118,19 @@ def test_from_dict_invalid_version(self, mocker):


class TestEndpoint:
def make_endpoint(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it probably makes more sense to either just make this a fixture or take advantage of setup_method to set self.endpoint to be the default endpoint constructed below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered a fixture, but you don't really gain anything here since it's not dependent on other fixtures. It's still going to call it every time, just with the overhead of pytest wrangling. setup_method could work, but I still hate it 😂. Totally unclear to most pytest users what that does IMO.

@dbanty dbanty enabled auto-merge (squash) April 5, 2021 19:14
@dbanty dbanty merged commit bf575fb into main Apr 5, 2021
@dbanty dbanty deleted the fix/template-duplicate-return-type branch April 5, 2021 19:21
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.

3 participants