Fix various issues reported by SonarCloud reports #2229
Conversation
| } else { | ||
| return "null"; | ||
| } | ||
| return "null"; |
There was a problem hiding this comment.
The code block aims to illustrate what type to check when computing the example value.
There was a problem hiding this comment.
@wing328 I don't get your point, the previous code block returns null in all cases. Can you please elaborate more?
There was a problem hiding this comment.
My point is that for someone new to the code base, they may have no clue what type to check and what function to leverage (e.g. ModelUtils.isIneger) to check the type.
The code is put there for onboarding new users.
There was a problem hiding this comment.
Maybe it can be reverted and a note can be added that the conditionals exist as examples for new contributors?
There was a problem hiding this comment.
I would put the conditionals as comment to enlighten new contributors and just leave the code return null. I personally prefer to have cleaner code and cover the unusual case in a comment
There was a problem hiding this comment.
I often agree with this approach. However, if any of the methods are renamed, comments are often missed and this can be confusing.
From another approach, new and existing contributors might not easily reason about what a single "null" result would mean here, while the explicit cases provide clear intention.
I think commenting on either way would be acceptable. We can always change later if people have questions.
| } else { | ||
| return "null"; | ||
| } | ||
| return "null"; |
There was a problem hiding this comment.
|
cc @OpenAPITools/generator-core-team as well |
|
For #788 |
|
Minor suggestion: next time you may want to create a new branch for the change. |
|
Sure.
…On Sun, 3 Mar 2019 at 04:12, Ramzi Maalej ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
<#2229 (comment)>
:
> - } else if (ModelUtils.isDateSchema(schema)) {
- return "null";
- } else if (ModelUtils.isDateTimeSchema(schema)) {
- return "null";
- } else if (ModelUtils.isNumberSchema(schema)) {
- return "null";
- } else if (ModelUtils.isIntegerSchema(schema)) {
- return "null";
- } else if (ModelUtils.isStringSchema(schema)) {
- return "null";
- } else if (ModelUtils.isObjectSchema(schema)) {
- return "null";
- } else {
- return "null";
- }
+ return "null";
I would put the conditionals as comment to enlighten new contributors and
just leave the code return null. I personally prefer to have cleaner code
and cover the unusual case in a comment
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2229 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA5BdN3bcMvQXE6iqKe5X-uxlQimgOBjks5vStsRgaJpZM4bPTy1>
.
|
wing328
added
Enhancement: General
Feature: Generator
Enhancement: Feature
and removed
Enhancement: General
labels
wing328
changed the title
Fix the following issues:
https://sonarcloud.io/project/issues?id=org.openapitools%3Aopenapi-generator-project&open=AWUi1WNYg-pHJSBwrBAz&resolved=false&types=BUG
https://sonarcloud.io/project/issues?id=org.openapitools%3Aopenapi-generator-project&open=AWUi1WPRg-pHJSBwrBKT&resolved=false&types=BUG
https://sonarcloud.io/project/issues?id=org.openapitools%3Aopenapi-generator-project&open=AWUi1WPRg-pHJSBwrBKU&resolved=false&types=BUG
https://sonarcloud.io/project/issues?id=org.openapitools%3Aopenapi-generator-project&open=AWUi1WPRg-pHJSBwrBMR&resolved=false&types=BUG
@wing328