Skip to content

Fix/sanitize-endpoints-tag-during-parsing #328

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

Conversation

p1-ra
Copy link
Contributor

@p1-ra p1-ra commented Feb 7, 2021

Title

Fix/sanitize-endpoints-tag-during-parsing

Description

During the parsing of a Document that have two endpoints respectively tagged with AMF Subscription Info (Document) and AmfSubscriptionInfo (Document). They are considered as two different endpoints by the parser. Later on during the building of the API these tags are sanitized and transformed to snake case and are considered to belonging to the same endpoint (expected behaviour imo).

It's lead to an error during the creation of the directory for it since the same directory is tried to be created twice.

  File "/home/xxxx/.local/pipx/venvs/openapi-python-client/lib/python3.8/site-packages/openapi_python_client/__init__.py", line 206, in _build_api
    tag_dir.mkdir()
  File "/usr/lib/python3.8/pathlib.py", line 1266, in mkdir
    self._accessor.mkdir(self, mode)
FileExistsError: [Errno 17] File exists: '/tmp/my-test-api-client/my_test_api_client/api/amf_subscription_info_document'

This PR will move the sanitize behaviour of the tag into parser. These two tags are now considered to belonging to the same endpoint tagged amf_subscription_info_document.

QA Notes & Product impact

Sanitize endpoints tag during parsing (transform them to snake case), thus two endpoint tagged with respectivily AMF Subscription Info (Document) and AmfSubscriptionInfo (Document) will be considered to belongs to the same endpoint tagged amf_subscription_info_document

@dbanty

@p1-ra p1-ra force-pushed the fix/sanitize-endpoints-tag-during-parsing branch from e87cd13 to d14f579 Compare February 7, 2021 16:02
@codecov
Copy link

codecov bot commented Feb 7, 2021

Codecov Report

Merging #328 (fecc4a7) into main (661de57) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #328   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1395      1394    -1     
=========================================
- Hits          1395      1394    -1     
Impacted Files Coverage Δ
openapi_python_client/__init__.py 100.00% <ø> (ø)
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 661de57...19edc2c. Read the comment docs.

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.

Thanks for the PR @p1-ra! Just a small change to the changelog.

Also just double checking - did you run task re to check if there are any changes to the golden master? I don't think there would be so you may have already done so, just want to be sure :)

@dbanty
Copy link
Collaborator

dbanty commented Feb 8, 2021

@emann the e2e tests in CI would have failed if it needed a task re

Nementon and others added 2 commits February 9, 2021 09:38
@p1-ra p1-ra force-pushed the fix/sanitize-endpoints-tag-during-parsing branch from bd49582 to 86fa00d Compare February 9, 2021 08:39
@p1-ra
Copy link
Contributor Author

p1-ra commented Feb 9, 2021

I've updated the changelog and rebased the branch on top of main

@emann emann enabled auto-merge (squash) February 12, 2021 16:56
@emann emann merged commit d535bd0 into openapi-generators:main Feb 12, 2021
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.

4 participants