Feature/1255 suffix+prefix for model name #2211
Conversation
|
@ePaul @jimschubert thanks for the PR. I'll review later today and let you guys know if I've any question. |
| @@ -82,4 +82,9 @@ | |||
|
|
|||
| public static enum MODEL_PROPERTY_NAMING_TYPE {camelCase, PascalCase, snake_case, original} | |||
|
|
|||
| public static final String MODEL_NAME_PREFIX = "modelNamePrefix"; | |||
| public static final String MODEL_NAME_PREFIX_DESC = "Prefix that will be prepended to all model names. Default is the empty string."; | |||
There was a problem hiding this comment.
I believe you also aware that another way to add the default value is .defaultValue in CliOption (example)
In this case, I'm fine with both approaches as the default is an empty string.
There was a problem hiding this comment.
That's true for CliOption, but I don't think it would work the same for maven/airline cli options, e.g.:
@Option(name = {"--model-name-suffix"},
title = "model name suffix",
description = CodegenConstants.MODEL_NAME_SUFFIX_DESC)
Airline, for example, only has allowedValues.
edit: probably better in this case to just not say there's a default since it doesn't matter to the user.
There was a problem hiding this comment.
Maybe better If the option is not given, nothing will be prepended. (and analogously for the suffix)?
| @@ -377,6 +380,7 @@ public String toParamName(String name) { | |||
|
|
|||
| @Override | |||
| public String toModelName(String name) { | |||
| name = super.toModelName(name); | |||
There was a problem hiding this comment.
Would there be an issue if we don't call super.toModelName, which is defined as follows?
public String toModelName(String name) {
return initialCaps(name);
}
There was a problem hiding this comment.
in this super method (DefaultCodegen.toModelName) I added the actual logic of adding suffix and prefix. Of course I could instead duplicate this here instead of calling the super method (like what Jim did in the C# version).
There was a problem hiding this comment.
Personally I prefer doing it in JavaCodegen toModelName as it would make my life easier reviewing changes. For example if someone changes the default toModelName, I've to go through all codegen to see if any one of them calls super.toModelName to involve the default one.
There was a problem hiding this comment.
I agree. I think it's better for generators to opt-in to the prefix/suffix logic than to reimplement the base method just to remove prefix/suffix.
There was a problem hiding this comment.
I think removing the prefix/suffix after the super method added it is not a good idea (if you don't want them, simply don't pass those parameters), but there might be the use case of using a different separator/naming convention (instead of the camelCase) for a programming language, which could be a reason to implement it in the subclass too.
So I would let it stay in the superclass method (which makes it available for all generators which don't override it), but implement it again in the JavaClientCodegen instead of calling super.toModelName(...).
There was a problem hiding this comment.
Done as mentioned above.
…refix + suffix. This is just the implementation in DefaultCodegen and some configurator stuff, the connection to command line arguments is still missing.
This implements the command line arguments `--model-name-prefix` and `--model-name-suffix` for all languages which do not override `DefaultCodegen.toModelName()`. This fixes most of swagger-api#1255. Connection to the maven plugin works with additional-properties, a more explicit parameter waits for swagger-api#2168.
…r C#. AbstractCSharpCodegen does override toModelName, which made the change to DefaultCodegen have no effect.
…x/Suffix addition. This allows using the model name prefix and suffix parameters also in Java generators. We add List and Map to the "language specific primitives" so they don't get mangled by the suffixes/prefixes in `getSwaggerType`
Instead of declaring `Map` and `List` as primitive (which caused them to be not imported anymore), now we exclude them from the model name transformation in `JavaClientCodegen.getSwaggerType`.
…dent from super implementation. As pointed out in the review, using a super.toModelName call makes future changes harder to review, therefore we are implementing the addition of suffix and prefix here again. In addition, I fixed the FIXME about assigning the parameter.
ePaul
force-pushed
the
feature/1255-suffix+prefix-for-model-name
branch
from
bf88ff6
to
36f7ffd
Compare
|
I rebased again to solve merge conflicts. @wing328 is it now fine this way or should I still change the parameter description text? |
|
@ePaul let me review now |
…del-name Feature/1255 suffix+prefix for model name
|
@ePaul @jimschubert thanks for the PR. For other langauges, we'll update them one by one and hopefully it will be ready before the 2.1.6 release. |
|
updated #1255 with a On Sat, Feb 27, 2016 at 12:33 AM, Jim Schubert notifications@github.com
|
This fixes most of #1255 by adding the options
--model-name-prefixand--model-name-suffixto the CLI, with corresponding changes to DefaultCodegen and CodegenConfigurator.For C# and Java generators we (@jimschubert and me) also added support code as they override
DefaultCodegen.toModelName.I guess this needs to be changed for quite some other languages too.
We also need some unit tests for this functionality, these are not yet included. (The existing unit tests still run without changes, as none of them uses these options.)