Skip to content

Issue 7638 #7790

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

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

Issue 7638 #7790

wants to merge 4 commits into from

Conversation

dkellenb
Copy link

@dkellenb dkellenb commented Mar 7, 2018

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: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

As described in Issue 7638 there is now the option to define the getter-prefix for Booleans. For those who favor isValid (or maybe hasValue) instead of getValid. Default is get.
Option example: --boolean-getter-prefix is or --boolean-getter-prefix has

@bbdouglas, @JFCote, @sreeshas, @jfiala, @lukoyanov, @cbornet, @jeff9finger: Please verify

I have have not changed the generation in AbstractCppCodegen which is still by default is. Same is true for SymfonyServerCodegen (PHP). Maybe it would be good to clean up these exceptional cases or keep it like i did in AbstractCppCodegen (C++).

Last remark: Note that it would even be better to have is-prefix for primitive boolean and get-prefix for Boolean type.

@@ -155,6 +156,11 @@ public void processOpts() {
this.setRemoveOperationIdPrefix(Boolean.valueOf(additionalProperties
.get(CodegenConstants.REMOVE_OPERATION_ID_PREFIX).toString()));
}

if (additionalProperties.containsKey(CodegenConstants.BOOLEAN_GETTER_PREFIX)) {
this.setRemoveOperationIdPrefix(Boolean.valueOf(additionalProperties
Copy link

Choose a reason for hiding this comment

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

Perhaps s/setRemoveOperationPrefix/setBooleanGetterPrefix/

@jeff9finger
Copy link

I assume that this new option will be available from the maven plugin as well.

If so, LGTM

@macjohnny
Copy link
Contributor

This PR resolves #7638

@cbornet
Copy link
Contributor

cbornet commented Mar 9, 2018

I don’t think this should be a global option. Most generators don’t care about it

@cbornet
Copy link
Contributor

cbornet commented Mar 9, 2018

I even don’t think this should be an option at all. Boolean wrapper should be called with get, not is. Always.

@JFCote
Copy link
Contributor

JFCote commented Mar 12, 2018

@cbornet @dkellenb To be honest, I think this is the same kind of debate that we could have for indentation style, tabs vs space, etc...
I think the more we provide ways for the user to customize the output of swagger-codegen, the better it is.

I agree with you that most generators don't care because they don't have the concept of getter/setter (think of typescript) or it's another concept completely (C# with properties) Maybe this should be implemented on a selected few generators (java mostly I think)? It could be in the AbstractJavaCodegen file?

As for always using "get" like @cbornet said, many people would disagree with him. Using "is" in the getter name help to see if the name of the variable is ok: https://stackoverflow.com/questions/5322648/for-a-boolean-field-what-is-the-naming-convention-for-its-getter-setter

I have projects who use both.

So maybe we should move this to the AbstractJavaCodegen as an option instead of a general option?

@macjohnny
Copy link
Contributor

I agree with @JFCote that it is better to have the getter naming customizable.
It is even more important since some property mappers do not map Boolean isMyVariable() but only boolean isMyVariable() or Boolean getMyVariable(), see e.g. in the Spring BeanWrapper: https://jira.spring.io/browse/SPR-9482
However, I would suggest to leave it in the default codegen, and not move it to the abstract java codegen, since the property-mapping issue could possibly also exist in other languages.

@cbornet
Copy link
Contributor

cbornet commented May 10, 2018

I think the more we provide ways for the user to customize the output of swagger-codegen, the better it is.

Sorry but I disagree. The more options there are, the harder it is to maintain, refactor, evolve, etc... I think we already have too many options on the Java gen which has become very messy (we should do something about that at some point btw).

As for always using "get" like @cbornet said, many people would disagree with him. Using "is" in the getter name help to see if the name of the variable is ok: https://stackoverflow.com/questions/5322648/for-a-boolean-field-what-is-the-naming-convention-for-its-getter-setter

This SOF link is about the boolean primitive, not the Boolean wrapper type. While it's OK to use the is getter for boolean, it's not allowed for the Boolean wrapper in the JavaBeans spec as @macjohnny pointed out.

@alex-spicer
Copy link

Was this never merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants