Skip to content

Conversation

@bjarnij
Copy link
Contributor

@bjarnij bjarnij commented Mar 12, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The generated code for discriminators that are not undefined discriminators, lacked quotes around the name of the property containing the discriminator causing the generated code not to compile.
Also I explicitly defined the undefined discriminator as any to get rid of the [ts] Member 'discriminator' implicitly has an 'any' type. error message from the TypeScript compiler when the compiler option noImplicitAny is set to true.

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny

bjarnij added 2 commits March 12, 2018 15:49
To get rid of the `[ts] Member 'discriminator' implicitly has an 'any' type.` error message from the TypeScript compiler when `noImplicitAny` is set to true.
Otherwise the code wouldn't work probably and the TypeScript compiler didn't even compile it without errors.
The Petstore client sample only contains undefined discriminators so this change does not affect the code generation of the Petstore client sample.
Copy link
Contributor

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

Looks good to me

{{/discriminator}}
{{^discriminator}}
static discriminator = undefined;
static discriminator : any = undefined;
Copy link
Contributor

@TiFu TiFu Mar 14, 2018

Choose a reason for hiding this comment

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

Could you please remove the space before :? Just to keep the code style consistent.

Would defining the type as string be correct? IIRC the discriminator is just a string denoting the property name. I would prefer a more specific type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TiFu Yes type string is correct. I've updated the PR to according to your comments.

@gbrown-ce
Copy link
Contributor

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @bjarnij

Any chance we can get this to merge into 2.4? Its blocking our ability to use discriminators in our current workflow.

Copy link
Contributor

@gbrown-ce gbrown-ce left a comment

Choose a reason for hiding this comment

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

Just some minor tweaks for CI purposes.

{{/discriminator}}
{{^discriminator}}
static discriminator = undefined;
static discriminator: string = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should be

static discriminator: string | undefined = undefined;

so that it can pass CI. The same with all the rest of the spots with undefined assignments.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bjarnij I checked this locally and it in fact does fix the issue:

{{#discriminator}}
static discriminator: string | undefined = "{{discriminator}}";
{{/discriminator}}
{{^discriminator}}
static discriminator: string | undefined = undefined;
{{/discriminator}}

@gbrown-ce
Copy link
Contributor

@wing328 I think you can close this PR now after merging this: #8157

@wing328 wing328 closed this May 10, 2018
@gbrown-ce
Copy link
Contributor

@wing328 do you know who is responsible for updating this? https://hub.docker.com/r/swaggerapi/swagger-codegen-cli/

I'd love to get this latest changed into that image.

@wing328
Copy link
Contributor

wing328 commented May 10, 2018

@gbrown-ce I'll look into that...

@gbrown-ce
Copy link
Contributor

@wing328 you're awesome, 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.

5 participants