-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[Swift5]: encode nil when swift5 optional is empty #10929
Closed
jarrodparkes
wants to merge
1
commit into
OpenAPITools:master
from
jarrodparkes:feat-10926/swift-5-encode-nil
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
generatorName: swift5 | ||
outputDir: samples/client/petstore/swift5/encodeModelNullProperties | ||
inputSpec: modules/openapi-generator/src/test/resources/2_0/swift/petstore-with-fake-endpoints-models-for-testing.yaml | ||
templateDir: modules/openapi-generator/src/main/resources/swift5 | ||
generateAliasAsModel: true | ||
additionalProperties: | ||
podAuthors: "" | ||
podSummary: PetstoreClient | ||
sortParamsByRequiredFlag: false | ||
encodeModelNullProperties: true | ||
projectName: PetstoreClient | ||
podHomepage: https://github.com/openapitools/openapi-generator |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
105 changes: 105 additions & 0 deletions
105
samples/client/petstore/swift5/encodeModelNullProperties/.gitignore
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
# Created by https://www.toptal.com/developers/gitignore/api/swift,xcode | ||
# Edit at https://www.toptal.com/developers/gitignore?templates=swift,xcode | ||
|
||
### Swift ### | ||
# Xcode | ||
# | ||
# gitignore contributors: remember to update Global/Xcode.gitignore, Objective-C.gitignore & Swift.gitignore | ||
|
||
## User settings | ||
xcuserdata/ | ||
|
||
## compatibility with Xcode 8 and earlier (ignoring not required starting Xcode 9) | ||
*.xcscmblueprint | ||
*.xccheckout | ||
|
||
## compatibility with Xcode 3 and earlier (ignoring not required starting Xcode 4) | ||
build/ | ||
DerivedData/ | ||
*.moved-aside | ||
*.pbxuser | ||
!default.pbxuser | ||
*.mode1v3 | ||
!default.mode1v3 | ||
*.mode2v3 | ||
!default.mode2v3 | ||
*.perspectivev3 | ||
!default.perspectivev3 | ||
|
||
## Obj-C/Swift specific | ||
*.hmap | ||
|
||
## App packaging | ||
*.ipa | ||
*.dSYM.zip | ||
*.dSYM | ||
|
||
## Playgrounds | ||
timeline.xctimeline | ||
playground.xcworkspace | ||
|
||
# Swift Package Manager | ||
# Add this line if you want to avoid checking in source code from Swift Package Manager dependencies. | ||
# Packages/ | ||
# Package.pins | ||
# Package.resolved | ||
# *.xcodeproj | ||
# Xcode automatically generates this directory with a .xcworkspacedata file and xcuserdata | ||
# hence it is not needed unless you have added a package configuration file to your project | ||
# .swiftpm | ||
|
||
.build/ | ||
|
||
# CocoaPods | ||
# We recommend against adding the Pods directory to your .gitignore. However | ||
# you should judge for yourself, the pros and cons are mentioned at: | ||
# https://guides.cocoapods.org/using/using-cocoapods.html#should-i-check-the-pods-directory-into-source-control | ||
# Pods/ | ||
# Add this line if you want to avoid checking in source code from the Xcode workspace | ||
# *.xcworkspace | ||
|
||
# Carthage | ||
# Add this line if you want to avoid checking in source code from Carthage dependencies. | ||
# Carthage/Checkouts | ||
|
||
Carthage/Build/ | ||
|
||
# Add this lines if you are using Accio dependency management (Deprecated since Xcode 12) | ||
# Dependencies/ | ||
# .accio/ | ||
|
||
# fastlane | ||
# It is recommended to not store the screenshots in the git repo. | ||
# Instead, use fastlane to re-generate the screenshots whenever they are needed. | ||
# For more information about the recommended setup visit: | ||
# https://docs.fastlane.tools/best-practices/source-control/#source-control | ||
|
||
fastlane/report.xml | ||
fastlane/Preview.html | ||
fastlane/screenshots/**/*.png | ||
fastlane/test_output | ||
|
||
# Code Injection | ||
# After new code Injection tools there's a generated folder /iOSInjectionProject | ||
# https://github.com/johnno1962/injectionforxcode | ||
|
||
iOSInjectionProject/ | ||
|
||
### Xcode ### | ||
# Xcode | ||
# gitignore contributors: remember to update Global/Xcode.gitignore, Objective-C.gitignore & Swift.gitignore | ||
|
||
|
||
|
||
|
||
## Gcc Patch | ||
/*.gcno | ||
|
||
### Xcode Patch ### | ||
*.xcodeproj/* | ||
!*.xcodeproj/project.pbxproj | ||
!*.xcodeproj/xcshareddata/ | ||
!*.xcworkspace/contents.xcworkspacedata | ||
**/xcshareddata/WorkspaceSettings.xcsettings | ||
|
||
# End of https://www.toptal.com/developers/gitignore/api/swift,xcode |
23 changes: 23 additions & 0 deletions
23
samples/client/petstore/swift5/encodeModelNullProperties/.openapi-generator-ignore
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# OpenAPI Generator Ignore | ||
# Generated by openapi-generator https://github.com/openapitools/openapi-generator | ||
|
||
# Use this file to prevent files from being overwritten by the generator. | ||
# The patterns follow closely to .gitignore or .dockerignore. | ||
|
||
# As an example, the C# client generator defines ApiClient.cs. | ||
# You can make changes and tell OpenAPI Generator to ignore just this file by uncommenting the following line: | ||
#ApiClient.cs | ||
|
||
# You can match any string of characters against a directory, file or extension with a single asterisk (*): | ||
#foo/*/qux | ||
# The above matches foo/bar/qux and foo/baz/qux, but not foo/bar/baz/qux | ||
|
||
# You can recursively match patterns against a directory, file or extension with a double asterisk (**): | ||
#foo/**/qux | ||
# This matches foo/bar/qux, foo/baz/qux, and foo/bar/baz/qux | ||
|
||
# You can also negate patterns with an exclamation (!). | ||
# For example, you can ignore all files in a docs folder with the file extension .md: | ||
#docs/*.md | ||
# Then explicitly reverse the ignore rule for a single file: | ||
#!docs/README.md |
110 changes: 110 additions & 0 deletions
110
samples/client/petstore/swift5/encodeModelNullProperties/.openapi-generator/FILES
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
.gitignore | ||
Cartfile | ||
Package.swift | ||
PetstoreClient.podspec | ||
PetstoreClient/Classes/OpenAPIs/APIHelper.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/AnotherFakeAPI.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/FakeAPI.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/FakeClassnameTags123API.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/PetAPI.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/StoreAPI.swift | ||
PetstoreClient/Classes/OpenAPIs/APIs/UserAPI.swift | ||
PetstoreClient/Classes/OpenAPIs/CodableHelper.swift | ||
PetstoreClient/Classes/OpenAPIs/Configuration.swift | ||
PetstoreClient/Classes/OpenAPIs/Extensions.swift | ||
PetstoreClient/Classes/OpenAPIs/JSONDataEncoding.swift | ||
PetstoreClient/Classes/OpenAPIs/JSONEncodingHelper.swift | ||
PetstoreClient/Classes/OpenAPIs/Models.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/AdditionalPropertiesClass.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Animal.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/AnimalFarm.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ApiResponse.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ArrayOfArrayOfNumberOnly.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ArrayOfNumberOnly.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ArrayTest.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Capitalization.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Cat.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/CatAllOf.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Category.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ClassModel.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Client.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Dog.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/DogAllOf.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/EnumArrays.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/EnumClass.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/EnumTest.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/File.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/FileSchemaTestClass.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/FormatTest.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/HasOnlyReadOnly.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/List.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/MapTest.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/MixedPropertiesAndAdditionalPropertiesClass.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Model200Response.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Name.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/NumberOnly.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Order.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/OuterComposite.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/OuterEnum.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Pet.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/ReadOnlyFirst.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Return.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/SpecialModelName.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/StringBooleanMap.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/Tag.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/TypeHolderDefault.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/TypeHolderExample.swift | ||
PetstoreClient/Classes/OpenAPIs/Models/User.swift | ||
PetstoreClient/Classes/OpenAPIs/OpenISO8601DateFormatter.swift | ||
PetstoreClient/Classes/OpenAPIs/SynchronizedDictionary.swift | ||
PetstoreClient/Classes/OpenAPIs/URLSessionImplementations.swift | ||
README.md | ||
docs/AdditionalPropertiesClass.md | ||
docs/Animal.md | ||
docs/AnimalFarm.md | ||
docs/AnotherFakeAPI.md | ||
docs/ApiResponse.md | ||
docs/ArrayOfArrayOfNumberOnly.md | ||
docs/ArrayOfNumberOnly.md | ||
docs/ArrayTest.md | ||
docs/Capitalization.md | ||
docs/Cat.md | ||
docs/CatAllOf.md | ||
docs/Category.md | ||
docs/ClassModel.md | ||
docs/Client.md | ||
docs/Dog.md | ||
docs/DogAllOf.md | ||
docs/EnumArrays.md | ||
docs/EnumClass.md | ||
docs/EnumTest.md | ||
docs/FakeAPI.md | ||
docs/FakeClassnameTags123API.md | ||
docs/File.md | ||
docs/FileSchemaTestClass.md | ||
docs/FormatTest.md | ||
docs/HasOnlyReadOnly.md | ||
docs/List.md | ||
docs/MapTest.md | ||
docs/MixedPropertiesAndAdditionalPropertiesClass.md | ||
docs/Model200Response.md | ||
docs/Name.md | ||
docs/NumberOnly.md | ||
docs/Order.md | ||
docs/OuterComposite.md | ||
docs/OuterEnum.md | ||
docs/Pet.md | ||
docs/PetAPI.md | ||
docs/ReadOnlyFirst.md | ||
docs/Return.md | ||
docs/SpecialModelName.md | ||
docs/StoreAPI.md | ||
docs/StringBooleanMap.md | ||
docs/Tag.md | ||
docs/TypeHolderDefault.md | ||
docs/TypeHolderExample.md | ||
docs/User.md | ||
docs/UserAPI.md | ||
git_push.sh | ||
project.yml |
1 change: 1 addition & 0 deletions
1
samples/client/petstore/swift5/encodeModelNullProperties/.openapi-generator/VERSION
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
5.3.0 |
7 changes: 7 additions & 0 deletions
7
...ft5/encodeModelNullProperties/.swiftpm/xcode/package.xcworkspace/contents.xcworkspacedata
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
1 change: 1 addition & 0 deletions
1
samples/client/petstore/swift5/encodeModelNullProperties/Cartfile
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
github "Flight-School/AnyCodable" ~> 0.6.1 |
22 changes: 22 additions & 0 deletions
22
samples/client/petstore/swift5/encodeModelNullProperties/Info.plist
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
<plist version="1.0"> | ||
<dict> | ||
<key>CFBundleDevelopmentRegion</key> | ||
<string>$(DEVELOPMENT_LANGUAGE)</string> | ||
<key>CFBundleExecutable</key> | ||
<string>$(EXECUTABLE_NAME)</string> | ||
<key>CFBundleIdentifier</key> | ||
<string>$(PRODUCT_BUNDLE_IDENTIFIER)</string> | ||
<key>CFBundleInfoDictionaryVersion</key> | ||
<string>6.0</string> | ||
<key>CFBundleName</key> | ||
<string>$(PRODUCT_NAME)</string> | ||
<key>CFBundlePackageType</key> | ||
<string>FMWK</string> | ||
<key>CFBundleShortVersionString</key> | ||
<string>1.0</string> | ||
<key>CFBundleVersion</key> | ||
<string>1</string> | ||
</dict> | ||
</plist> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@jarrodparkes if something
isNullable
shouldn't we callcontainer.encodeNil
?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.
Maybe this should be
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.
What do you think? This is more a question, than an answer, so I would like to have your feedback on this 🙂
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.
@4brunu Your code sample looks right to me, but the problems I seemed to be encountering were weird compound states with
required
andisNullable
. In fact, these two booleans have confused me (and other devs on my team) quite a bit since they seem to refer to the same thing depending on context.What I was using as my "source of truth" was regenerate the sample and make sure all optionals are wrapped with the
if/let
construct ORencodeIfPresent
. If the resulting code ever tried toencode
an optional, then it wouldn't even compile.The code I currently seemed to cover these cases well, but completely open for improvement/change. I can try your suggestion and see if the resulting code looks good.
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.
required
means if is must be present, either the value or null.isNullable
defines if the type can be null.https://stackoverflow.com/questions/45575493/what-does-required-in-openapi-really-mean
This makes me rethink the entire PR.
Instead of an addition, this should be a bug fix, and we don't need the new flag
encodeModelNullProperties
.I think this should be the implementation for all the cases.
What do you think?
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.
But if it's required, it should always be encoded, even if it's null.
If it's not required, it should be encoded only if it's present, or am I wrong?
So I think the current implementation is correct, or am I mistaken?
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 we are both saying the same thing and the implementation is wrong.
translated
But if it is
{{#required}}
, then it should always be encoded, even if it's null. ==>encode
If it's
{{^required}}
, then it should be encoded only if it's present ==>encodeIfPresent
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.
We are saying the same thing 🙂
That's the current implementation on master branch
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 I'm going crazy :face_palm: you're right. I think I was probably just using the combination of
required
andx-nullable
wrong this whole timeThere 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's normal, it's confusing the required vs nullable thing.