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

Add support for tags and include responses without content #924

Merged
merged 8 commits into from
Dec 20, 2022

Conversation

Aedial
Copy link
Contributor

@Aedial Aedial commented Nov 16, 2022

The exact implementation of responses without content is still to be decided

The exact implementation of responses without content is still to be decided
@Aedial
Copy link
Contributor Author

Aedial commented Nov 16, 2022

There are 2 versions of parse_responses(), the legacy one that doesn't return any empty response and parse_responses_with_none(), which returns the empty responses alongside the others.
I ran the tests, and both seem to work against the test suite, but some dependents might rely on the specific behavior of the legacy one.
Adding include_empty_models directly to parse_responses() would have been tempting, but it would break the signature that seems to define the parse_*() functions (and breaks inheritance on dependents).

@Aedial
Copy link
Contributor Author

Aedial commented Nov 16, 2022

Merging this PR would solve koxudaxi/fastapi-code-generator#114, koxudaxi/fastapi-code-generator#176 and koxudaxi/fastapi-code-generator#207 (if I understood these correctly).
I already have working code for these issue, waiting for this PR to go through.

@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (e404bca) compared to base (e9b6edf).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##            master      #924     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files           11        21     +10     
  Lines         1020      2603   +1583     
  Branches       201       614    +413     
===========================================
+ Hits          1020      2603   +1583     
Flag Coverage Δ
unittests 99.92% <99.90%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
datamodel_code_generator/model/__init__.py 100.00% <ø> (ø)
datamodel_code_generator/__init__.py 100.00% <100.00%> (ø)
datamodel_code_generator/__main__.py 100.00% <100.00%> (ø)
datamodel_code_generator/format.py 100.00% <100.00%> (ø)
datamodel_code_generator/http.py 100.00% <100.00%> (ø)
datamodel_code_generator/imports.py 100.00% <100.00%> (ø)
datamodel_code_generator/model/base.py 100.00% <100.00%> (ø)
datamodel_code_generator/model/enum.py 100.00% <100.00%> (ø)
...atamodel_code_generator/model/pydantic/__init__.py 100.00% <100.00%> (ø)
...amodel_code_generator/model/pydantic/base_model.py 100.00% <100.00%> (ø)
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@koxudaxi
Copy link
Owner

@Aedial
Thank you for creating the PR.
Should we add the option to call the new methods?

@Aedial
Copy link
Contributor Author

Aedial commented Nov 22, 2022

@koxudaxi So, if I understand well, the "right" way would be to add another parameter to OpenAPIParser (datamodel_code_generator/parser/openapi.py#L244), generate (datamodel_code_generator/init.py#L200), and arg_parser (datamodel_code_generator/main.py#L349) ? I was fearing contributing to uncontrolled organic growth going this route, but it seems it is fine by you.

@koxudaxi
Copy link
Owner

I think we can add tags to the enum for the option. '--openapi-scopes'

arg_parser.add_argument(
'--openapi-scopes',
help='Scopes of OpenAPI model generation (default: schemas)',
choices=[o.value for o in OpenAPIScope],
nargs='+',
default=[OpenAPIScope.Schemas.value],
)

class OpenAPIScope(Enum):
Schemas = 'schemas'
Paths = 'paths'

I was fearing contributing to uncontrolled organic growth going this route, but it seems it is fine by you.
I don't find a good solution to avoid the growth now 😅
If some want the option for none content response option, I will think about it again.

@koxudaxi
Copy link
Owner

Do we need tags' logic in datamodel-code-generator?
We can write the method in fastapi-code-generator.

@Aedial
Copy link
Contributor Author

Aedial commented Nov 23, 2022

Do we need tags' logic in datamodel-code-generator? We can write the method in fastapi-code-generator.

Putting the tags handling in fastapi-codegen makes things awkward, as it would mean putting a special clause for tags in parse_operation (fastapi-code-generator/parser.py), while keeping it in datamodel-code-generator stays consistent with the rest (parse_parameters, parse_request_body, parse_responses) and keeps the logic of Operation in one place. From a "separation of responsibilities" point of view, it feels cleaner.

@Aedial
Copy link
Contributor Author

Aedial commented Nov 29, 2022

Just realized I never committed the tests (oops)

@koxudaxi
Copy link
Owner

@Aedial
I'm sorry for my late reply.
I misunderstood. We don't need the new arguments of CLI.
We should add the argument for the parser.

@koxudaxi koxudaxi merged commit 82e44e4 into koxudaxi:master Dec 20, 2022
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