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

Feature/typescript fetch/api config class name #7916

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feature/typescript fetch/api config class name #7916

wants to merge 4 commits into from

Conversation

jawa-the-hutt
Copy link

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 @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny

Description of the PR

Implements suggested changes for typescript-fetch client that allows you to change the default name of the Configuration class used to setup access to an API. This would close issue #7911

This commit adds another system property named apiConfigClassName By adding this property, it will allow you to change the name of this class so as to avoid confusion with template generated classes and interfaces that also are named Configuration. When passing in the new name, you can use camelCase or PascalCase strings and the Mustache Lambda feature will know when to upper/lowercase generated class names and references.

You can activate this property via this method:

java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar generate \
-i http://petstore.swagger.io/v2/swagger.json \
-l typescript-fetch -o samples/client/petstore/typescript-fetch/builds/api-config-class-name \
-D apiConfigClassName=apiConfiguration

If this additional property is not passed in then the class name will default to Configuration.

@jawa-the-hutt
Copy link
Author

So, there is something I need to fix on this before it can be merged. will update accordingly

.gitignore Outdated
@@ -34,6 +34,8 @@ packages/
*.xml~
*.t~

Copy link
Contributor

Choose a reason for hiding this comment

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

special character? should be removed

Copy link
Author

Choose a reason for hiding this comment

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

Not sure. I'm doing all of my development via VS Code in the supplied Docker container. When it mounts the container into the local path it appears that the /.m2/repository folder showed up with the folder name having this special character. Git tried to add all the files in it into source control. So, I excluded it in my .gitignore.

Open to suggestions as to how to avoid this situation, but I haven't found a solution for it yet.

Copy link
Author

Choose a reason for hiding this comment

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

FYI, here's how it appears in VS Code:

capture

Copy link
Author

@jawa-the-hutt jawa-the-hutt Mar 27, 2018

Choose a reason for hiding this comment

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

Wanted to update this with an issue I just opened up: #7928

This appears to be an issue with Docker for Windows and a line in the run-in-docker.sh script. I've found what I believe is a work around, and will update this PR so that I can take out the special character from .gitignore.

@@ -76,7 +76,7 @@ export class RequiredError extends Error {
* {{&description}}{{/description}}
* @export
*/
export const {{classname}}FetchParamCreator = function (configuration?: Configuration) {
export const {{classname}}FetchParamCreator = function ({{#lambda.lowercase}}{{apiConfigClassName}}{{/lambda.lowercase}}?: {{#lambda.titlecase}}{{apiConfigClassName}}{{/lambda.titlecase}}) {
Copy link
Contributor

@macjohnny macjohnny Mar 27, 2018

Choose a reason for hiding this comment

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

why need to change the parameter name? it makes the template harder to read...

Copy link
Author

@jawa-the-hutt jawa-the-hutt Mar 27, 2018

Choose a reason for hiding this comment

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

I'll reply here on this even though this specific line is getting ready to change when I make a commit here in a few minutes. lambda.lowercase is going to change to lambda.camelcase_param.

I agree it does make the template a bit harder to read, but the parameter name is changing to this as it will now be dynamic in nature based on what you pass into the generator.

That's the basic point of this pull request. There are swagger definitions that may auto-generate other parameters/variables named configuration in the same class. When this happens you end up with invalid code as you can't have two parameters/variables with the same name.

Copy link
Author

Choose a reason for hiding this comment

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

@macjohnny, if you haven't yet, take a look at #7911. I include a sample swagger definition where if it's run against the generator, you will end up with multiple parameters/variables named configuration.

I would also add that the reason I'm putting the lambda.camelcase_param is to try and conform to standard Javascript/Typescript patterns that has Class/Interface names as PascalCase and parameters/variables in camelCase. By using the lambda's, I'm allowing someone to pass in the dynamic Configuration class name as something like ApiConfiguration or apiConfiguration and the template will then know when to use PascalCase or camelCase automatically based on those standards.

@jawa-the-hutt
Copy link
Author

I have another set of changes I'd like to merge into the project that deal with solutions specifically for #7595, but could also be a pattern that is used in addressing #3620

It adds another system property option when calling the generator that allows you to set a flag that puts all the parameters in an object like it used to in 2.2.3 and below, or just leaves it the default way parameters are generated now from 2.3.0 and above. I've gotten the changes coded and done but the code includes the code from this pull request in it. I can open another pull request, but it would essentially supersede this one.

So, my point is should I wait for this pull request to be fully merged and then submit the other pull request, or should I add those changes into this pull request and we get both at the same time?

@macjohnny
Copy link
Contributor

@jawa-the-hutt I think you should open a new PR with the changes for #7595, while leaving this one open, too. That way, the code can be reviewed and tested in parallel.

@jawa-the-hutt
Copy link
Author

Ok, PR #7930 has been created with the other set of changes.

Fixed new line characters in scripts
Fixed camelCase issue in api.mustache
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