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 uniqueNames option. #151

Merged
merged 8 commits into from May 10, 2018
Merged

Add uniqueNames option. #151

merged 8 commits into from May 10, 2018

Conversation

fabiandev
Copy link
Contributor

This is an approach to handle duplicate type names via the API, while preserving backward compatibility through the newly introduced option uniqueNames which defaults to false. Fixes #112.

@domoritz
Copy link
Collaborator

Can you add a test?

@domoritz
Copy link
Collaborator

I don't understand the test. It just tests whether string constants work, doesn't it?

@fabiandev
Copy link
Contributor Author

fabiandev commented Oct 14, 2017

It tests whether different types of the same name can be obtained with the uniqueNames option. I've just added the string literal to easily see that both types MyObject from main.ts and other.ts can be generated. Since there are other tests that actually test the JSON schemas, this one should verify that the newly introduced option uniqueNames works alongside the getSymbols method.

@domoritz
Copy link
Collaborator

Thank you! Shouldn't this option be the default? Could you rebase against the latest master?

@fabiandev
Copy link
Contributor Author

Yes, probably this option could be default, but I wanted this pull request to be backward compatible. I will revisit this PR and will also resolve the conflicts.

@domoritz
Copy link
Collaborator

Thank you @fabiandev

@domoritz
Copy link
Collaborator

domoritz commented May 4, 2018

Could you rebase the branch so that I can merge it?

Copy link
Collaborator

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Looks great except for one question.

@@ -854,6 +866,16 @@ export class JsonSchemaGenerator {
return root;
}

public getSymbols(): SymbolRef[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this method do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought it may be more clear to use function overloading here. Could also be removed, since the implementation with the optional parameter should do.

@fabiandev fabiandev changed the title Added uniqueNames option. Add uniqueNames option. May 10, 2018
@domoritz domoritz merged commit d08931c into YousefED:master May 10, 2018
@domoritz
Copy link
Collaborator

Thank you!

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