Skip to content

fix: Fail when duplicate names are generated - #336

Merged
dbanty merged 9 commits into
openapi-generators:mainfrom
benchling:forest-clash
Feb 10, 2021
Merged

fix: Fail when duplicate names are generated#336
dbanty merged 9 commits into
openapi-generators:mainfrom
benchling:forest-clash

Conversation

@forest-benchling

@forest-benchlingforest-benchling commented Feb 10, 2021•
edited
Loading

Copy link
Copy Markdown
Collaborator

Previously, it would silently create buggy code if duplicate models were generated with the same name; see #335 and #323.

@codecov

codecovBot commented Feb 10, 2021•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #336 (ba9bf86) into main (2833602) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@ Coverage Diff @@## main #336 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 47 47 Lines 1390 1393 +3 =========================================
+ Hits 1390 1393 +3
Impacted FilesCoverage Δ
...penapi_python_client/parser/properties/__init__.py100.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 2833602...3ae0f18. Read the comment docs.

@forest-benchling

forest-benchling commented Feb 10, 2021

Copy link
Copy Markdown
CollaboratorAuthor

cc @dbanty@emann@bowenwr@packyg

dbanty
dbanty requested changes Feb 10, 2021
View reviewed changes
Comment threadopenapi_python_client/parser/properties/__init__.py Outdated
additional_properties=additional_properties,
)
if prop.reference.class_name in schemas.models:
raise NameClashException(f'Attempted to generate duplicate models with name "{prop.reference.class_name}"')

@dbantydbantyFeb 10, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should return a PropertyError here instead so our higher level error handling code can manage it rather than printing a stack trace.

@forest-benchlingforest-benchlingFeb 10, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

👍. Thanks for the review!

dbanty
dbanty requested changes Feb 10, 2021
View reviewed changes

@dbantydbanty left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you also add unit test checks for these so Codecov is happy? And sync up with main which contains #334 now.

@forest-benchling

forest-benchling commented Feb 10, 2021

Copy link
Copy Markdown
CollaboratorAuthor

@dbanty I removed the enum check, since it turns out that IIUC we were already checking for enum name collisions (see openapi_python_client/parser/properties/__init__.py:349).

@dbanty

dbanty commented Feb 10, 2021

Copy link
Copy Markdown
Collaborator

Awesome, thanks! Will merge this one now.

@dbanty
dbanty merged commit 2bfe610 into openapi-generators:mainFeb 10, 2021
dbanty added a commit that referenced this pull request Feb 10, 2021
@dbanty
docs: Add CHANGELOG entry for#336
968d614
@forest-benchling
forest-benchling deleted the forest-clash branch February 10, 2021 21:31
@dbantydbanty mentioned this pull request Apr 4, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dbantydbantydbanty approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@forest-benchling@dbanty