-
Notifications
You must be signed in to change notification settings - Fork 475
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
fixes #398 and adds more tests to prove #399 #428
fixes #398 and adds more tests to prove #399 #428
Conversation
@@ -329,7 +333,7 @@ export class TypeResolver { | |||
|
|||
if (typeNode.kind === ts.SyntaxKind.ArrayType) { | |||
const arrayType = typeNode as ts.ArrayTypeNode; | |||
return this.getAnyTypeName(arrayType.elementType) + '[]'; | |||
return this.getAnyTypeName(arrayType.elementType) + 'Array'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WoH this is the change that I thought could be breaking for some users if they were relying on getting back GenericRequestTypeAliasModel1[]
they would now get GenericRequestTypeAliasModel1Array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But I have more of these, if you're not sure about breaking changes, I think we can make it worth the 3.0 release 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WoH would I be overthinking it if I added a swagger.config parameter called noSymbolsInRefNames
that would allow @Asterix112 (and anyone else who has stricter needs for their ref names) to opt into this?
And then when we get to version 3.0 we can remove this noSymbolsInRefNames
.
The reason why I'm resistant to going to a 3.0 version right now is (a) Asterix appears to need this soon and (b) there are a lot of great features that we can release without going to 3.0 (like your union PR, and the PR that fixes auto controller generation).
I appreciate any and all thoughts. Thank you for this collaboration @WoH. It's a lot of fun! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ ] in the $ref wasn't ever valid, was it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, you are correct @WoH. It gives this error in SwaggerEditor: ref values must be RFC3986-compliant percent-encoded URIs
.
So we're safe to make this change since it's a true bug fix.
}, | ||
genericNested2: (propertyName, propertySchema) => { | ||
expect(propertySchema.$ref).to.eq('#/components/schemas/GenericRequestArrayTypeAliasModel2'); | ||
genericNestedArrayKeyword1: (propertyName, propertySchema) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WoH I had to rename genericNested2
to differentiate between the []
tests. I also added genericMultiNested
to see if we would have an issue with multiple levels of generics. Turns out that we don't because your function is recursive. Nice work! :)
… we have extensive unit test coverage there.
fixes #398 and adds more tests to prove #399