Skip to content

fix!: Normalize generated module names to allow more tags [#428]. Thanks @iamnoah! #448

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 5, 2021
Merged

fix!: Normalize generated module names to allow more tags [#428]. Thanks @iamnoah! #448

merged 5 commits into from
Jul 5, 2021

Conversation

dbanty
Copy link
Collaborator

@dbanty dbanty commented Jul 4, 2021

This PR is motivated to fix #428 but as part of that fix there are a few other changes:

  1. More consistently name generated modules, may be a breaking change
  2. Update mypy and add some now-separate typing modules (mostly just to increase refactor confidence)
  3. Remove the last runtime global (field prefix)
  4. Refactor impacted tests to be less fragile for the future

@dbanty dbanty changed the title fix!: Normalize generated module names to allow more tags [#428] fix!: Normalize generated module names to allow more tags [#428]. Thanks @iamnoah! Jul 4, 2021
@dbanty dbanty requested a review from emann July 4, 2021 19:00
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2021 •
edited
Loading

Codecov Report

Merging #448 (d679f49) into main (f19b582) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #448   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1563      1562    -1     
=========================================
- Hits          1563      1562    -1
Impacted Files Coverage Δ
openapi_python_client/config.py 100.00% <ø> (ø)
openapi_python_client/utils.py 100.00% <ø> (ø)
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/property.py 100.00% <100.00%> (ø)
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 f19b582...d679f49. Read the comment docs.

@dbanty
Copy link
Collaborator Author

dbanty commented Jul 4, 2021

@forest-benchling if you get a chance can you take a look at this? Fairly simple changes but a lot of code is touched.

forest-benchling
forest-benchling previously approved these changes Jul 5, 2021
View reviewed changes
Copy link
Collaborator

@forest-benchling forest-benchling 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 the ping @dbanty. AFAICT, we don't have any tags that would be affected. cc @GitOnUp

end_to_end_tests/custom-templates-golden-record/my_test_api_client/api/__init__.py
@@ -19,3 +20,7 @@ def default(cls) -> Type[DefaultEndpoints]:
@classmethod
def parameters(cls) -> Type[ParametersEndpoints]:
return ParametersEndpoints

@classmethod
def tag1(cls) -> Type[Tag1Endpoints]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat! cc @bowenwr @GitOnUp on this feature, of using the tags to define class methods on the client. This is just like what we've done on our wrapper client with the "services".

openapi_python_client/parser/properties/property.py Outdated
from ... import Config
from ...utils import fix_keywords, fix_reserved_words, sanitize, snake_case

_PythonIdentifier = NewType("_PythonIdentifier", str)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be underscored if it's being used by a bunch of other modules?

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 believe it's only being used directly by one other module. The _ is really there to indicate that you shouldn't be constructing this class directly, but only by using the sanitization method. Best I could come up with for trying to enforce sanitization with types 🤷.

Although now that I'm thinking about it, maybe using a custom __init__ would be better... what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, cool solution you ended up with.

openapi_python_client/parser/properties/property.py Outdated
_PythonIdentifier = NewType("_PythonIdentifier", str)


def to_valid_python_identifier(*, value: str, prefix: str) -> _PythonIdentifier:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this moved out of utils with all the other case conversion logic? Seems clearer to be there, IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To keep the private constructor as private as possible, since the type is used directly here.

openapi_python_client/parser/openapi.py Outdated
@@ -41,7 +42,7 @@ def from_data(
operation: Optional[oai.Operation] = getattr(path_data, method)
if operation is None:
continue
tag = utils.snake_case((operation.tags or ["default"])[0])
tag = to_valid_python_identifier(value=(operation.tags or ["default"])[0], prefix="tag")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be too much work to make "tag" configurable, like field prefix?

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 think it'd be pretty easy to add in a future PR if anyone wants it.

@dbanty dbanty force-pushed the 428-generated-package-names-start-with-numbers-if-tag-starts-with-a-number branch from 6bdd232 to a761eff Compare July 5, 2021 19:45
@dbanty
Copy link
Collaborator Author

dbanty commented Jul 5, 2021

FYI @forest-benchling I did end up changing it to a public class with a custom __new__ since that definitely seems more clear, thanks for the push!

@dbanty dbanty merged commit 457ebca into main Jul 5, 2021
@dbanty dbanty deleted the 428-generated-package-names-start-with-numbers-if-tag-starts-with-a-number branch July 5, 2021 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@forest-benchling forest-benchling forest-benchling left review comments

@emann emann Awaiting requested review from emann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

generated package names start with numbers if tag starts with a number
3 participants
@dbanty @codecov-commenter @forest-benchling