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

[Angular-typescript] Regression/bug: Parameters with an _ ignore naming convention #7262

Closed
HermenOtter opened this issue Dec 27, 2017 · 8 comments

Comments

@HermenOtter
Copy link

HermenOtter commented Dec 27, 2017

Description

Parameters with an _ ignore naming convention since a few days. This bug occurred a few months ago as well and has been fixed at the time.

Swagger-codegen version

master(54ce4ca and 8ae1184) are not working as expected.

Swagger declaration file content or url
/sadr/v1/soorten_adres/{code_soort_adres}:
    parameters:
    - name: "code_soort_adres"
      in: "path"
      required: true
      type: "string"
Command line used for generation

java -jar swagger-codegen-cli.jar generate -i https://api.swaggerhub.com/apis/Centric/Belastingen_Subjecten/0.2.4 -l typescript-angular -c=./SCGConf.md

SCGConf.md
{"modelPropertyNaming": "original"}

Code generation
Generates code_soort_adres as parameter for the method, but when used in that same method it is called as codeSoortAdres which obviously doesn't work.

public getSoortenAdresByCode(code_soort_adres: string,
return this.httpClient.get<any>(${this.basePath}/.../${encodeURIComponent(String(codeSoortAdres))}

Related issues/PRs

7201, regression

Suggest a fix/enhancement

I believe it is has something to do with issue 7201.

@wing328 @JFCote @kenisteward

@meisterlampe
Copy link

meisterlampe commented Jan 3, 2018

I have the same problem after upgrading to v2.3.0
The generated code isn't compilable

@JFCote
Copy link
Contributor

JFCote commented Jan 4, 2018

@HermenOtter @meisterlampe I'm the one responsible for this regression (I think). It was fixing other stuff and it is working like the other generators but there seems to be something missing. I can assure you that it was tested against our petstore samples and it was tested by a user who had problem with that.

Anyway, I will have a look at this today and try to come up with a fix as soon as possible. I'll keep you updated here.

@JFCote
Copy link
Contributor

JFCote commented Jan 4, 2018

@HermenOtter @meisterlampe
I have debugged this and I am relieved to see that my code is not responsible for this regression. This problem is created by this very weird loop:
https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/TypeScriptAngularClientCodegen.java#L268

As you can see, underscores are deleted and the camelCase naming convention is used by default, which is completely wrong I think. This explains why underscores disappear from the parameter name.

I will create a PR to fix it but I think it would be good to have the advice of @defmonk0 to understand why it was done that way.

So, PR coming very soon with what I think is a pretty good fix.

@JFCote
Copy link
Contributor

JFCote commented Jan 4, 2018

@HermenOtter @meisterlampe There you go. Can you try the PR and see if it fix your problem? Your feedback appreciated.

@defmonk0
Copy link
Contributor

defmonk0 commented Jan 5, 2018

Hey everyone. To explain the idea behind this, it helps to see the original issue I opened that prompted these changes.

The general idea is this: Angular's standard style guide uses camel case for more or less everything. As such, codegen generates the files using this by default as well, and so function parameters are converted. Because the return statements in the service files use the URI path rather than those parameters, I made it so the variables in the path would be changed as well. In all, the exact opposite of what you're experiencing now.

At the time of change, I was unaware the parameter names were configurable via modelPropertyNaming. The code changes are therefore forcing the change to camel case in the return statement when you may not want them to. I was not using this option, so the current code makes sense in that case. If it is standard for this to be configurable across all of the languages codegen supports, then changing that code block to execute appropriately based on the config would be ideal.

EDIT: To explain the loop itself, it was originally done before my changes much more simply with a single regex. I originally was going to keep using regex to find and convert the variable names, but it was complicated and very difficult to understand when looking at it. It was much simpler to iterate through the string myself, which is what you see there.

@JFCote
Copy link
Contributor

JFCote commented Jan 5, 2018

@defmonk0 Thanks for the clarification.

With that in mind, can you do a review of my proposed PR and see if it makes sense? Thanks!

wing328 pushed a commit that referenced this issue Jan 7, 2018
…that camelCase naming was forced only in this part of the code when everywhere else it is configurable. (#7313)
@meisterlampe
Copy link

I just tested this using a Snapshot of 2.4.0 and it is working perfectly fine. Thanks for the fast reaction and solution!

@wing328
Copy link
Contributor

wing328 commented Jan 8, 2018

@meisterlampe thanks for confirming it's no longer an issue with the latest SNAPSHOT version of master (2.3.1)

Thanks @JFCote again for the quick fix 👍

@HermenOtter please give it another try with the latest SNAPSHOT

@wing328 wing328 closed this as completed Jan 8, 2018
@wing328 wing328 added this to the v2.3.1 milestone Jan 8, 2018
jimschubert added a commit to jimschubert/swagger-codegen that referenced this issue 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

No branches or pull requests

5 participants