-
Notifications
You must be signed in to change notification settings - Fork 243
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
feat(dotnet): handling optional and variadic parameters #680
Changes from 2 commits
9cbe4d5
0a9b489
69dd8a8
7bd0db9
3c8ef6c
9369104
621e07f
8d49901
8f9d65e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,26 +229,20 @@ export class DotNetGenerator extends Generator { | |
this.code.openBlock(`public${inner}${absPrefix} class ${className}${implementsExpr}`); | ||
|
||
// Compute the class parameters | ||
// TODO: add the support for optional parameters | ||
// Not achievable as of today and not supported by the current generator | ||
// https://github.com/awslabs/jsii/issues/210 | ||
let parametersDefinition = ''; | ||
let parametersBase = ''; | ||
const initializer = cls.initializer; | ||
if (initializer) { | ||
this.dotnetDocGenerator.emitDocs(initializer); | ||
this.dotnetRuntimeGenerator.emitDeprecatedAttributeIfNecessary(initializer); | ||
if (initializer.parameters) { | ||
parametersDefinition = this.renderParametersString(initializer.parameters); | ||
for (const p of initializer.parameters) { | ||
const pType = this.typeresolver.toDotNetType(p.type); | ||
const isOptionalPrimitive = this.isOptionalPrimitive(p) ? '?' : ''; | ||
if (parametersDefinition !== '') { | ||
parametersDefinition += ', '; | ||
parametersBase += `${this.nameutils.convertParameterName(p.name)}`; | ||
// If this is not the last parameter, append , | ||
if (initializer.parameters.indexOf(p) !== initializer.parameters.length - 1) { | ||
parametersBase += ', '; | ||
} | ||
parametersDefinition += `${pType}${isOptionalPrimitive} ${this.nameutils.convertParameterName(p.name)}`; | ||
parametersBase += `${this.nameutils.convertParameterName(p.name)}`; | ||
|
||
} | ||
} | ||
|
||
|
@@ -443,18 +437,36 @@ export class DotNetGenerator extends Generator { | |
} | ||
} | ||
|
||
// TODO: I uncovered an issue with optional parameters and ordering them | ||
// They are currently not supported by the generator. | ||
// In C#, optional parameters need to be declared after required parameters | ||
// We could make changes to the parameters ordering in the jsii model to support them | ||
// But then an optional parameter becoming non optional would create a mess | ||
// https://github.com/awslabs/jsii/issues/210 | ||
/** | ||
* Renders method parameters string | ||
*/ | ||
private renderMethodParameters(method: spec.Method): string { | ||
return this.renderParametersString(method.parameters); | ||
} | ||
|
||
/** | ||
* Renders parameters string for methods or constructors | ||
*/ | ||
private renderParametersString(parameters: spec.Parameter[] | undefined): string { | ||
const params = []; | ||
if (method.parameters) { | ||
for (const p of method.parameters) { | ||
const isOptionalPrimitive = this.isOptionalPrimitive(p) ? '?' : ''; | ||
const st = `${this.typeresolver.toDotNetType(p.type)}${isOptionalPrimitive} ${this.nameutils.convertParameterName(p.name)}`; | ||
if (parameters) { | ||
for (const p of parameters) { | ||
let optionalPrimitive = ''; | ||
let optionalKeyword = ''; | ||
let type = this.typeresolver.toDotNetType(p.type); | ||
if (p.optional) { | ||
if (this.isOptionalPrimitive(p)) { | ||
optionalPrimitive = '?'; | ||
optionalKeyword = ' = null'; | ||
} else { | ||
optionalKeyword = ' = null'; | ||
} | ||
} | ||
if (p.variadic) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this an else if? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like JSII is already doing the "mutual exclusion", but no harm in enforcing it with an if/else There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah actually TypeScript won't allow you to use both the variadic syntax and the optional syntax at the same time... So if it's variadic, it cannot be optional, and vice-versa. But there's definitely no harm in checking again. |
||
type = `params ${type}[]`; | ||
} | ||
const st = | ||
`${type}${optionalPrimitive} ${this.nameutils.convertParameterName(p.name)}${optionalKeyword}`; | ||
params.push(st); | ||
} | ||
} | ||
|
@@ -637,7 +649,6 @@ export class DotNetGenerator extends Generator { | |
let isVirtualKeyWord = ''; | ||
// If the prop parent is a class | ||
if (cls.kind === spec.TypeKind.Class) { | ||
|
||
const implementedInBase = this.isMemberDefinedOnAncestor(cls as spec.ClassType, prop); | ||
if (implementedInBase || datatype || proxy) { | ||
// Override if the property is in a datatype or proxy class or declared in a parent class | ||
|
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.
The embedded if and else both set optionalKeyword to the same. Move this set to the parent if.
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.
Oops, this is a refactoring leftover. Removing