Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[C#] Remove null checks for C# value types #2933

Merged
merged 1 commit into from
May 30, 2019
Merged

[C#] Remove null checks for C# value types #2933

merged 1 commit into from
May 30, 2019

Conversation

etherealjoy
Copy link
Contributor

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, ./bin/openapi3/{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\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Remove null checks on C# value types which throws a lot of warnings.

Null checks on enums which is a value type are already avoided. Combining simple types and enum as value types and avoiding null checks.
See here.
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/value-types

@wing328 @mandrean @jimschubert

@auto-labeler
Copy link

auto-labeler bot commented May 19, 2019

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

@etherealjoy
Copy link
Contributor Author

I will regenerate code if it looks OK

etherealjoy
etherealjoy commented May 19, 2019
View reviewed changes
...penapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCSharpCodegen.java
@@ -454,6 +461,19 @@ private void postProcessEnumRefs(final Map<String, Object> models) {
var.isPrimitiveType = true;
}
}
for (CodegenProperty var : model.vars) {
Copy link
Contributor Author

@etherealjoy etherealjoy May 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Question:
why is this line (some lines above) not covering all enums

for (CodegenProperty var : model.allVars)

I had to add a separate one for this below

for (CodegenProperty var : model.vars) {

Copy link
Member

Choose a reason for hiding this comment

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

It should. That could be a bug with allVars.

I'll look into it in the coming week.

@etherealjoy etherealjoy force-pushed the csharp-remove-valueType-nullcheck branch from 8250b07 to 4f05900 Compare May 19, 2019 00:53
@etherealjoy
Copy link
Contributor Author

etherealjoy commented May 19, 2019 •
edited
Loading

I think I need to add extra condition if it is a valueType but not nullable, however I see parsing of the nullable field is not done.
On the other hand I am checking the dataType so at this point if a nullable type is specified it won't be a valueType anymore.

@wing328
Copy link
Member

wing328 commented May 19, 2019

You mean something like {{#isNullable}} ... {{/isNulalble}}?

@etherealjoy
Copy link
Contributor Author

Exactly. So if the value is nullable it will not be e.g. int but int?. But in this case if the dataType is used for postprocess it is not anymore a valueType
I will try out some specs with nullable fields.

@wing328
Copy link
Member

wing328 commented May 19, 2019

I think nullable is supported in the C# generators already: https://github.com/OpenAPITools/openapi-generator/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Amerged+nullable+csharp

but you will need to use x-nullable in OAS v2 instead which does not support nullable natively.

@etherealjoy
Copy link
Contributor Author

Thank you. I see that the simple type is already changed to nullable type. Then this will be covered.
I will make the test with this.

@etherealjoy
Copy link
Contributor Author

Ok, I verified it. the nullable types are having null checks. only value Types are skipped.

@wing328
Copy link
Member

wing328 commented May 21, 2019

Can you please update the C# netcore sample as well?

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/docs/EnumTest.md
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/ApiResponse.cs
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/Cat.cs
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/CatAllOf.cs
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/Category.cs
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/EnumTest.cs
	modified:   samples/client/petstore/csharp-netcore/OpenAPIClient/src/Org.OpenAPITools/Model/FormatTest.cs

@etherealjoy etherealjoy force-pushed the csharp-remove-valueType-nullcheck branch 2 times, most recently from 6820342 to 1c8dbb3 Compare May 22, 2019 16:57
@etherealjoy etherealjoy force-pushed the csharp-remove-valueType-nullcheck branch from 1c8dbb3 to 261cee2 Compare May 28, 2019 09:26
@wing328 wing328 merged commit aae9763 into OpenAPITools:master May 30, 2019
@wing328 wing328 added this to the 4.0.1 milestone May 31, 2019
@wing328 wing328 changed the title [C#]Remove null checks for C# value types [C#] Remove null checks for C# value types May 31, 2019
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 5, 2019
@jimschubert
Merge branch '4.1.x' into configurator
232131e 
* 4.1.x: (56 commits)
  sync master
  Update compatibility table
  Ruby client: escape path parameters (OpenAPITools#3039)
  [gradle plugin] Release 4.0.1 fixes (OpenAPITools#3051)
  Update version to 4.0.2-SNAPSHOT (OpenAPITools#3047)
  Map number to double time since float is also parsed as double in Qt5 C++ (OpenAPITools#3046)
  Prepare 4.0.1 release (OpenAPITools#3041)
  [gradle] Reworking publishing pipeline (OpenAPITools#2886)
  [typescript-fetch] Fix uploading files (OpenAPITools#2900)
  Resolves OpenAPITools#2962 - Add properties config to Maven parameters (OpenAPITools#2963)
  fix(golang): Check error of xml Encode (OpenAPITools#3027)
  [C++][Restbed] Add handler callback methods (OpenAPITools#2911)
  Remove null checks for C# value types (OpenAPITools#2933)
  [python-server] Support python 3.7 for all server-generators (OpenAPITools#2884)
  Use golang's provided method names (gin) (OpenAPITools#2983)
  [python] Adding constructor parameters to Configuration and improving documentation (OpenAPITools#3002)
  Add new option to maven plugin's readme (OpenAPITools#3025)
  Engine param in maven plugin. (OpenAPITools#2881)
  Added support for patterns on model properties (OpenAPITools#2948)
  [csharp] Make API response headers dictionary case insensitive (OpenAPITools#2998)
  ...
@etherealjoy etherealjoy deleted the csharp-remove-valueType-nullcheck branch September 16, 2019 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 left review comments

Assignees
No one assigned
Labels
Client: C-Sharp Enhancement: General Server: C-Sharp
Projects
None yet
Milestone
4.0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@etherealjoy @wing328