[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[typescript] Append enum suffix without model suffix - #5138

Merged
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix
Jan 31, 2020
Merged

[typescript] Append enum suffix without model suffix#5138
macjohnny merged 11 commits into
OpenAPITools:masterfrom
crunchbase:typescript-angular-enum-suffix

Conversation

@amakhrov

@amakhrovamakhrov commented Jan 29, 2020

Copy link
Copy Markdown
Contributor

Fixes#5135

I was not able to approach this problem without introducing backward-incompatible changes.
A new enumSuffix option is introduced to minimize migration efforts, though.

UPDATE: actually, backward compatibility is now ensured by having v4-compat value by default for the newly introduced option

  • (breaking change) With modelNameSuffix, only append "Enum" suffix but not model suffix. So instead of ResponseModelPropNameModelEnum we now get ResponseModelPropNameEnum.
  • Without modelNameSuffix, all typescript generators except typescript-angular keep the existing behavior (emit smth like ResponsePropNameEnum)
  • (breaking change) typescript-angular now follows the same enum naming rules as other TS generators. It still uses its custom modelSuffix, though - so it's still possible to configure it via modelNameSuffix or modelSuffix. Both options should work the same with the updated enum naming schema.
  • enumSuffix is now configurable via options. This would provide smoother migration for existing users (e.g. one can set it to "ModelEnum" to mimic the old behavior).
  • (bonus) More robust model names generation. Previously it would be possible to have a spec with a model named error (note lowcase!), which would then emit a Typescript interface Error. This should not have been possible, because Error is a builtin class. Now all the sanity checks are done to a fully-transformed model (after being camelized, and adding prefix and suffix). This addresses a part of the problem described in [BUG][typescript] Models named after language-specific types generate incorrectly #5139)

PR checklist

  • Read the contribution guidelines.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

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

@auto-labeler

Copy link
Copy Markdown

👍 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.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is a mapping for the OAS2 built-in file type. It's similar to the existing mapping defined in TypeScriptAngularClientCodegen.java:

typeMapping.put("file", "Blob");

Without that, the updated toModelName() logic would add a bogus import for api endpoints that deal with type:file (imageUpload endpoint in PetStore)

Frankly, I'm not sure if the old mapping in line 151 here is needed at all.

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov Thanks a lot for your PR.
If I understand it correctly, the problem is not a bug itself, but rather a naming inconvenience?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

@macjohnny i guess it depends on a point of view.
I consider it a bug, because it makes modelSuffix rather unusable in typescript-angular, resulting in enum names that are called Models (assuming Model is the suffix used).

However, one can argue that since generator still emits some valid typescript, it's not a bug but a feature.

If you think it's rather a feature than a bugfix - do you think it's still valuable to be merged into the next major version?

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

Thinking further about introducing backward incompatible changes.... Perhaps there is a way to keep compatibility by introducing a special v4-compat value of this new enumNameSuffix option, and making it a default. This value will enforce the existing behavior for all generators. This value can then be removed in the major release if needed.
Any thoughts?

@macjohnny

Copy link
Copy Markdown
Member

@amakhrov I think it is a good idea to have a backwards-compatible behavior by default, so we can merge it and include it in the next patch release

@macjohnny

Copy link
Copy Markdown
Member

concerning bug or feature, since the code compiles as it is generated now, i would consider it a feature/improvement of the naming, which is, of course, favorable, too.

@amakhrov

Copy link
Copy Markdown
ContributorAuthor

All right, so I'll make the change described (introduce a special v4-compat value for this option), and update the PR - all tomorrow (it's past midnight over here already)

@macjohnnymacjohnny added this to the 4.2.3 milestone Jan 30, 2020
@amakhrov
amakhrovforce-pushed the typescript-angular-enum-suffix branch from 3f19d54 to 130c8ffCompareJanuary 30, 2020 23:32
…r-enum-suffix
# Conflicts:
#	docs/generators/javascript-flowtyped.md
#	docs/generators/typescript-angular.md
#	docs/generators/typescript-angularjs.md
#	docs/generators/typescript-aurelia.md
#	docs/generators/typescript-axios.md
#	docs/generators/typescript-fetch.md
#	docs/generators/typescript-inversify.md
#	docs/generators/typescript-jquery.md
#	docs/generators/typescript-node.md
#	docs/generators/typescript-redux-query.md
#	docs/generators/typescript-rxjs.md
#	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java
#	modules/openapi-generator/src/test/java/org/openapitools/codegen/options/TypeScriptAureliaClientOptionsProvider.java

@macjohnnymacjohnny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@wing328wing328 modified the milestones: 4.2.3, 5.0.0, 4.3.0Jan 31, 2020
@macjohnny
macjohnny merged commit e32a2f0 into OpenAPITools:masterJan 31, 2020
@amakhrov
amakhrov deleted the typescript-angular-enum-suffix branch February 7, 2020 00:27
@florianstuerzlinger

Copy link
Copy Markdown

Is it possible to not add a suffix to enums?
i tried calling the generator with an empty parameter -
This leads to the suffix "False". Any solution for this?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][typescript][typescript-angular] enumSuffix appends to modelNameSuffix

4 participants

@amakhrov@macjohnny@florianstuerzlinger@wing328