Skip to content

[feature] Add option to disable stripping of common prefix enum #5166

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

Merged

Conversation

johnflanigan
Copy link
Contributor

@johnflanigan johnflanigan commented Jan 30, 2020

PR detail

  • Add enableEnumPrefixRemoval option to toggle whether enum prefixes are stripped. Currently the prefixes are stripped so I have this option set to true by default.
  • Add tests to DefaultCodegenTest to test functionality and verify default behavior has not changed.

PR checklist

  • [x ] Read the contribution guidelines.
  • [ x] If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • [ x] 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).
  • [ x] File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • [ x] Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @ircecho @swipesight @jaumard @nickmeinhold @athornz @amondnet

I'm not very familiar with open source contributing so I'll apologize in advance if anything is not correct. Happy to address any problems or things I should do differently.

@auto-labeler
Copy link

auto-labeler bot commented Jan 30, 2020

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

@amakhrov
Copy link
Contributor

Looks like a couple of similar boolean options that exist in codegen use slightly different naming: REMOVE_OPERATION_ID_PREFIX, STRIP_PACKAGE_NAME, etc
What do you think about making this new option to follow the same pattern? I.e. REMOVE_ENUM_PREFIX. Or, for better clarity, even REMOVE_ENUM_VALUE_PREFIX

@johnflanigan johnflanigan force-pushed the add-enum-prefix-removal-flag branch from b2abfa0 to 5c59a89 Compare January 31, 2020 03:44
@johnflanigan
Copy link
Contributor Author

Sure, that change makes sense to me. I've updated the PR to use REMOVE_ENUM_VALUE_PREFIX.

@johnflanigan
Copy link
Contributor Author

I noticed DartClientCodegen and PythonClientExperimentalCodegen follow a similar pattern of calling findCommonPrefixOfVars to find the common prefix and then remove it. Should this change be made there as well to keep behavior consistent across languages?

@johnflanigan
Copy link
Contributor Author

The same pattern to create enumVars appeared to be used in multiple places so I extracted it into a method called buildEnumVars. The only place that seemed slightly different was in the DartClientCodegen where isString wasn't set in the map so I'm hoping that adding it doesn't break anything Dart specific.

@amakhrov
Copy link
Contributor

@johnflanigan nice job on getting rid of duplicate code!

Looks like an important thing missing in the PR is that it doesn't @mention the technical committee - so chances are the maintainers won't notice it.

@johnflanigan
Copy link
Contributor Author

Thanks, good point! I didn't mention anyone initially since it wasn't targeting a specific language, but I'll copy the Dart technical committee since its making a change to the DartClientCodegen now.

@johnflanigan johnflanigan force-pushed the add-enum-prefix-removal-flag branch from aee108c to 2136239 Compare February 1, 2020 20:08
@johnflanigan johnflanigan force-pushed the add-enum-prefix-removal-flag branch from 2136239 to af4b3bb Compare February 4, 2020 17:13
@johnflanigan
Copy link
Contributor Author

Is anyone available to review this change?

cc OpenAPI Generator Core Team: @wing328 @jimschubert @cbornet @ackintosh @jmini @etherealjoy

@jimschubert
Copy link
Member

I think this looks good. I'll give it a little longer for review from others on the core team due to the new getter/setter on CodegenConfig.

@jimschubert jimschubert added this to the 4.3.0 milestone Feb 8, 2020
@jimschubert jimschubert self-assigned this Feb 8, 2020
@jimschubert
Copy link
Member

Thanks for the PR. There doesn't seem to be anymore comments, so I'm merging.

@jimschubert jimschubert merged commit c6ad35c into OpenAPITools:master Feb 8, 2020
@air237
Copy link

air237 commented Feb 19, 2020

I'd like to set the removeEnumValuePrefix to false in the pom like this:

<plugin>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-maven-plugin</artifactId>
    <version>4.3.0-SNAPSHOT</version>
    <configuration>
        <removeEnumValuePrefix>false</removeEnumValuePrefix>

The property value haven't changed, the prefix is still trimmed.
How can I set it correctly?

@johnflanigan
Copy link
Contributor Author

@air237 Can you try setting it in additionalProperties under configuration. I'm not too familiar with the Maven plugin but I think it would be something like this based on the plugin's README:

<configuration>
    <additionalProperties>removeEnumValuePrefix=false</additionalProperties>
</configuration>

I just tested using the Gradle plugin which I'm more familiar with and this worked:

openApiGenerate {
    generatorName = "java"
    inputSpec = "$rootDir/pet-store.yaml".toString()
    additionalProperties = [
            removeEnumValuePrefix: "false"
    ]
}

@johnflanigan
Copy link
Contributor Author

@air237 Were you able to get it working? If there are problems with it, I'm happy to look into it further!

@air237
Copy link

air237 commented Feb 21, 2020

@johnflanigan Thank you for your prompt reply. I was able to get it working. I just tested it. Thanks!

MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
@wing328
Copy link
Member

wing328 commented Mar 27, 2020

@johnflanigan thanks for the PR, which has been included in the 4.3.0 release: https://twitter.com/oas_generator/status/1243455743937789952

@unexist
Copy link

unexist commented Oct 7, 2020

I wonder, why is this issue circularly linked without any proper docs? Is it really fixed right now? Version? What is correct syntax to prevent this behaviour?

Edit: I found a way that works properly for me, use the code from above inside of the plugin config - not execution config:

<plugin>
        <!-- Snip -->
    <configuration>
        <additionalProperties>removeEnumValuePrefix=false</additionalProperties>
    </configuration>
    <executions>
        <execution>
            <!-- Snip -->
            <configuration>
                <!-- Snip -->
            </configuration>
        </execution>
    </execution>
</plugin>

@stevegoossens
Copy link

@unexist thanks for posting the working maven plugin config for using removeEnumValuePrefix !

@gl0b3
Copy link

gl0b3 commented Oct 11, 2023

It's not working for me in version 3. Has it been ported?

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.

[REQ] Option to disable stripping of common enum prefix
8 participants