-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Fix issue 18889,15274 - configOptions to not remove enum prefix and oneOf subtypes annotations example #18998
base: master
Are you sure you want to change the base?
Conversation
removeEnumValuePrefixOpts.put("false", | ||
"No changes to the enum's are made."); | ||
removeEnumValuePrefixOpts.put("true", | ||
"With this option disabled, each enum will have the common prefix between them removed. This is the default option."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not: disabled -> enabled? Since this is when it's true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not understand your question. This option was implemented by someone else and that's how it ended up, I just wrote what happens with each option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's with the option enabled, each enum will have the common prefix between them removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. It was done like this. But it's been like this since Feb 8, 2020. PR link merged #5166
@@ -40,6 +40,7 @@ public class BashClientOptionsProvider implements OptionsProvider { | |||
= "false"; | |||
public static final String PREPEND_FORM_OR_BODY_PARAMETERS_VALUE = "true"; | |||
public static final String ENUM_UNKNOWN_DEFAULT_CASE_VALUE = "false"; | |||
public static final String REMOVE_ENUM_VALUE_PREFIX_VALUE = "true"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you adding this here if you're referencing the default codegen key?
My understanding is that these are for generator specific keys (which you aren't using)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was breaking the tests saying that this new property needed to be added. I just followed the ENUM_UNKNOWN_DEFAULT_CASE_VALUE example to solve the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this need to be defined here to just added to the builder below using .put(CodegenConstants.REMOVE_ENUM_VALUE_PREFIX, REMOVE_ENUM_VALUE_PREFIX_VALUE)
?
This definition (BashClientCodegen.REMOVE_ENUM_VALUE_PREFIX_VALUE
) isn't being used or referenced anywhere
Which tests was failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -47,6 +48,7 @@ public Map<String, String> createOptions() { | |||
.put(CodegenConstants.LEGACY_DISCRIMINATOR_BEHAVIOR, "true") | |||
.put(CodegenConstants.DISALLOW_ADDITIONAL_PROPERTIES_IF_NOT_PRESENT, "true") | |||
.put(CodegenConstants.ENUM_UNKNOWN_DEFAULT_CASE, ENUM_UNKNOWN_DEFAULT_CASE_VALUE) | |||
.put(CodegenConstants.REMOVE_ENUM_VALUE_PREFIX, REMOVE_ENUM_VALUE_PREFIX_VALUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did none of the samples change for any of these generators when you added the option?
Do they properly support this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new option was implemented in DefaultCodegen and has been in use for a long time.
My intention here is not to develop a new feature but to make its configuration available via configOptions so that devs can find this option more easily.
I just received test errors and resolved them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - although ideally we don't need to replicate each option in each sub-generator and can just refer to the global option
@@ -40,6 +40,7 @@ public class BashClientOptionsProvider implements OptionsProvider { | |||
= "false"; | |||
public static final String PREPEND_FORM_OR_BODY_PARAMETERS_VALUE = "true"; | |||
public static final String ENUM_UNKNOWN_DEFAULT_CASE_VALUE = "false"; | |||
public static final String REMOVE_ENUM_VALUE_PREFIX_VALUE = "true"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this need to be defined here to just added to the builder below using .put(CodegenConstants.REMOVE_ENUM_VALUE_PREFIX, REMOVE_ENUM_VALUE_PREFIX_VALUE)
?
This definition (BashClientCodegen.REMOVE_ENUM_VALUE_PREFIX_VALUE
) isn't being used or referenced anywhere
Which tests was failing?
I agree. However, the project is very big and I'm still getting to know it. For now, I will continue as is being done as I believe it is safer. In the future maybe I will be a little more daring. |
An option was implemented to not remove the common prefix between enums, but it was not available in the documentation and does not work by adding it to configOptions.
I just added it to cliOptions so that it appears in the documentation and works through configOptions in the pom.
merged: [feature] Add option to disable stripping of common prefix enum #5166
fix [BUG][JAVA] Enum not generated correctly for contents having common prefix #18889
I created a test to show how using oneOf with mapping works without duplicating class mappings.
fix [BUG][Spring] oneOf keyword generates wrong subtype annotations. #15274
example
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming 7.6.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @wing328