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

Support "duplicated" type definitions #1874

Closed
wants to merge 3 commits into from

Conversation

K-Phoen
Copy link

@K-Phoen K-Phoen commented Feb 20, 2024

This PR is an attempt at solving #666

The approach taken here is heavily inspired by #667, of course updated to work against the next branch.

From a high level, the following changes were introduced

  • when duplicated definitions are identified, we lift the ambiguity by generating new names for the definitions. This is done in the SchemaGenerator and mainly by the unambiguousName() function
  • since we need relevant information to generate decent new names, some parser components were extended to give us access to the file name from which definitions originate. For conflicting definitions, the non-common parts of the file names are used as "disambiguated names"

Storing the file name for relevant types is done in the first commit, the second one does the duplicate definitions detection + disambiguation.

if (!isLocalRef($ref)) {
result.$ref = $ref;
} else {
// THE Money Shot.
Copy link
Author

Choose a reason for hiding this comment

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

The only actual work performed by this function is here, in the next 3 lines.

Everything else is just recursively traversing the schema.

@K-Phoen K-Phoen changed the title Support *duplicated* type definitions Support duplicated type definitions Feb 20, 2024
@K-Phoen K-Phoen changed the title Support duplicated type definitions Support "duplicated" type definitions Feb 20, 2024
@domoritz
Copy link
Member

I think I am hesitant to accept the added complexity (regexes etc) right now. If you can find a simpler solution, I would be happy to consider it, though. I'll close this for now since the base has moved too much to merge this anyway.

@domoritz domoritz closed this Apr 23, 2024
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