Skip to content
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

[Typescript][Angular] fix regression/bug that parameters with an _ ignore naming convention #7313

Merged
merged 1 commit into from
Jan 7, 2018

Conversation

JFCote
Copy link
Contributor

@JFCote JFCote commented Jan 4, 2018

The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable.

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.

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09)

Description of the PR

Fix issue #7262

This bug seems to have been introduced by this commit: a63e3f1
So I'm tagging its author for review: @defmonk0

In summary, I've made sure that the parameters names in the path received the same treatment everywhere else instead of applying a camelCase by default.

…that camelCase naming was forced only in this part of the code when everywhere else it is configurable.
@defmonk0
Copy link
Contributor

defmonk0 commented Jan 5, 2018

The issue I see with this change is it essentially reverts to the original functionality before a63e3f1 (it would simply use the loop instead of the original regex method). This would just cause the issue I was attempting to fix.

As described in my response to issue #7262, it seems we need to change this code to be compatible with the modelPropertyNaming option. The functionality of this option is described in #6632.

My change is compatible when the option is not set (defaults to camelCase, I believe, per Angular style guide). This change is compatible when set to original, as described in #7262. We are completely missing compatibility with the option set to PascalCase.

Pinging @wing328, since he merged my changes initially and is active on every mention of this option I can find. ;P I imagine he'll have a better notion of how we're supposed to be supporting this, since I didn't even know it existed before now. Lol.

@JFCote
Copy link
Contributor Author

JFCote commented Jan 5, 2018

@defmonk0 My change is compatible with the naming option since it uses toVarName which is used in all other generators to take care of this:
See here: https://github.com/swagger-api/swagger-codegen/pull/7313/files#diff-ad60e100e050d5ef52943803a2adc5fdR279

By calling this, it will check what is the prefered naming convention and replace the param name by the good naming in the path.

@defmonk0
Copy link
Contributor

defmonk0 commented Jan 6, 2018

@JFCote Holy crap, I completely missed that function call nested inside the .append. My bad. Lol.

With that there, yes, I agree, this should work perfectly well. Looks fine to me.

@wing328 wing328 added this to the v2.3.1 milestone Jan 7, 2018
@wing328
Copy link
Contributor

wing328 commented Jan 7, 2018

@JFCote thanks for the quick fix.

@defmonk0 thanks for reviewing the change.

@wing328 wing328 merged commit 0a9c6f5 into swagger-api:master Jan 7, 2018
@wing328 wing328 changed the title Fix issue #7262 [Typescript][Angular] fix regression/bug that parameters with an _ ignore naming convention Jan 7, 2018
jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Jan 10, 2018
* master: (26 commits)
  [Scala] Fix async helper methods when body is optional (swagger-api#7274)
  [Rust] Recommend style based on 'rustfmt' defaults (swagger-api#7335)
  [Java:vertx] Initialize router in init method and re-use router member to create S… (swagger-api#7234)
  [Scala] Fix missing json4s import (swagger-api#7271)
  deploy snapshot version 2.3.1
  [Ada] Add Ada support for server code generator swagger-api#6680 (swagger-api#7256)
  add shijinkui to scala technical committee
  Generate swagger yaml for go client (swagger-api#7281)
  use openjdk7 in travis to ensure it works with jdk7
  docs(readme): update link to contributing guid (swagger-api#7332)
  Fix a regression bug that was introduce in a recent commit. Removed the tabs that were causing error in Play Framework (swagger-api#7241)
  Fix issue swagger-api#7262 with the parameter name in the path. The problem was that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (swagger-api#7313)
  Java8 fix (swagger-api#7260)
  update to 2.3.1-SNAPSHOT
  fix typo, update 2017 to 2018
  [Doc] add huawei cloud to companies list swagger-api#7308 (swagger-api#7309)
  Adding Peatio opensource as reference project (swagger-api#7267)
  Update README.md (swagger-api#7298)
  Update README.md (swagger-api#7299)
  [all] sys props in CodegenConstants
  ...
Sign up for free to 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.

3 participants