Skip to content

[typescript-node] fixed enum generator for top level enums#2266

Merged
macjohnny merged 5 commits intoOpenAPITools:masterfrom
spring-media:master
Jul 15, 2019
Merged

[typescript-node] fixed enum generator for top level enums#2266
macjohnny merged 5 commits intoOpenAPITools:masterfrom
spring-media:master

Conversation

@christof-vollrath-spring-media
Copy link
Copy Markdown
Contributor

@christof-vollrath-spring-media christof-vollrath-spring-media commented Feb 28, 2019 •
edited
Loading

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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

@auto-labeler
Copy link
Copy Markdown

auto-labeler Bot commented Feb 28, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@wing328 wing328 added this to the 4.0.0 milestone Mar 7, 2019
Closed
@jmini
Copy link
Copy Markdown
Member

jmini commented Mar 24, 2019

I have merged master into this PR and I have removed the Copyright 2018 SmartBear Software line

@jmini
Copy link
Copy Markdown
Member

jmini commented Mar 25, 2019

Dear TypeScript Technical Committee, can you have a look at this PR?

@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce

@mdaouas mdaouas mentioned this pull request Apr 4, 2019
Open
macjohnny
macjohnny reviewed Apr 11, 2019
View reviewed changes
Comment thread modules/openapi-generator/src/main/resources/typescript-node/model.mustache
export enum {{classname}} {
{{#allowableValues}}
{{#enumVars}}
{{name}} = <any> {{{value}}}{{^-last}},{{/-last}}
Copy link
Copy Markdown
Member

@macjohnny macjohnny Apr 11, 2019

Choose a reason for hiding this comment

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

is there any particular reason for using <any> here?
it seems to work out without it e.g. here

openapi-generator/modules/openapi-generator/src/main/resources/typescript-fetch/modelEnum.mustache

Lines 6 to 12 in b797662

export enum {{classname}} {
{{#allowableValues}}
{{#enumVars}}
{{{name}}} = {{{value}}}{{^-last}},{{/-last}}
{{/enumVars}}
{{/allowableValues}}
}

Copy link
Copy Markdown
Contributor Author

@christof-vollrath-spring-media christof-vollrath-spring-media Jul 15, 2019

Choose a reason for hiding this comment

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

I just copied this part from the already existing code for enums inside a request. "" was already used in the first typescript node template from Geoff Brown.
Being not a typescript expert myself I just didn't want to change too much.
And I just asked my typescript college, he thinks is not needed...

Comment thread ...va/org/openapitools/codegen/typescript/typescriptnode/TypescriptNodeEnumIntegrationTest.java
import java.util.HashMap;
import java.util.Map;

public class TypescriptNodeEnumIntegrationTest extends AbstractIntegrationTest {
Copy link
Copy Markdown
Member

@macjohnny macjohnny Apr 11, 2019

Choose a reason for hiding this comment

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

what does this test do?
to simply compare the generated files it would be better to add a new variation to https://github.com/OpenAPITools/openapi-generator/blob/master/bin/typescript-node-petstore-all.sh

Copy link
Copy Markdown
Contributor Author

@christof-vollrath-spring-media christof-vollrath-spring-media Jul 15, 2019

Choose a reason for hiding this comment

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

AbstractIntegrationTest are generally disabled at the moment (disabled not by me, and I have not understood why).
To test my implementation, I temporarily activated integration test.
And I am hoping that integration tests will be active in the future.
The petstore example doesn't use enums.
And I agree an example with enums would be create.

@wing328 wing328 modified the milestones: 4.0.0, 4.0.1 May 13, 2019
@wing328 wing328 modified the milestones: 4.0.1, 4.0.2 May 31, 2019
@wing328 wing328 modified the milestones: 4.0.2, 4.0.3 Jun 20, 2019
@wing328 wing328 modified the milestones: 4.0.3, 4.1.0 Jul 9, 2019
@alecf
Copy link
Copy Markdown

alecf commented Jul 11, 2019

@christof-vollrath-spring-media ping... I just came across this issue and was excited to see a fix... but it sounds like it still needs work?

@christof-vollrath-spring-media
Copy link
Copy Markdown
Contributor Author

christof-vollrath-spring-media commented Jul 15, 2019

@christof-vollrath-spring-media ping... I just came across this issue and was excited to see a fix... but it sounds like it still needs work?

For us, everything looked fine with this fix.
At least it's better than what we have at the moment, which is just wrong.

In the end we stopped using the Typescript client because of other issues concerning inheritance for incoming data.
Maybe we will come back to it in the future.

@macjohnny
Copy link
Copy Markdown
Member

macjohnny commented Jul 15, 2019

@christof-vollrath-spring-media thanks for your feedback. which issues concerning inheritance to you refer to?
could you please update with the latest master, so we could merge this one. Also, is the <any> typecast really necessary?

@macjohnny macjohnny merged commit 8417c5b into OpenAPITools:master Jul 15, 2019
@wing328 wing328 changed the title [typescript-node] fixed enum generator for top level enums #665 [typescript-node] fixed enum generator for top level enums Aug 9, 2019
@wing328
Copy link
Copy Markdown
Member

wing328 commented Aug 10, 2019

@christof-vollrath-spring-media thanks for the PR, which has been included in the 4.1.0 release: https://twitter.com/oas_generator/status/1160000504455319553

@bodograumann bodograumann mentioned this pull request Sep 28, 2020
Merged
6 tasks
macjohnny pushed a commit that referenced this pull request Sep 28, 2020
@bodograumann
[typescript] Fix generation of enum models (#7529)
54dd185 
This fixes #665 for the consolidated typescript generator.
Original fix for typescript-node was in PR #2266, merged as
8417c5b in version 4.1.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wing328 wing328 wing328 left review comments

@macjohnny macjohnny macjohnny left review comments

Assignees

No one assigned

Labels

Client: TypeScript Feature: Enum Issue: Bug

Projects

None yet

Milestone

4.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@christof-vollrath-spring-media @jmini @alecf @macjohnny @wing328 @christof-vollrath