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

[Swift4] Allow for custom dateformatter to be used #6672

Merged
merged 6 commits into from
Nov 13, 2017
Merged

[Swift4] Allow for custom dateformatter to be used #6672

merged 6 commits into from
Nov 13, 2017

Conversation

DawidvanGraan
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 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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

Setting a custom date format to be used on decoding.

My server response date uses the format YYYY-MM-DD'T'HH:mm:ss.sssZ, I needed a way to set DateFormatter on the decoding of the data models. Suggest allowing for a custom DateFormat to be used in CodableHelper.

Usage:

// Custom Date
let dateFormat = DateFormatter()
dateFormat.dateFormat = "YYYY-MM-DD'T'HH:mm:ss.sssZ" // "2017-10-12T17:03:21.000Z"
CodableHelper.dateformatter = dateFormat

Suggested code change in CodableHelper

// Allow for custom date formatter to be used
open static var dateformatter: DateFormatter?

// Check if formatter is set, else fallback to default values
let decoder = JSONDecoder()
if let df = self.dateformatter {
    decoder.dateDecodingStrategy = .formatted(df)
} else {
    decoder.dateDecodingStrategy = .deferredToDate
    if #available(iOS 10.0, *) {
        decoder.dateDecodingStrategy = .iso8601
    }
}

Also changed the dateDecodingStrategy from '.base64' to '.deferredToDate` from issue #6576.

@DawidvanGraan DawidvanGraan changed the title Issue 6576 [Swift4] Allow for custom dateformatter to be used Oct 13, 2017
@wing328
Copy link
Contributor

wing328 commented Oct 16, 2017

@DawidvanGraan thanks for the PR.

cc @jgavris @ehyche @Edubits @jaz-ah

@wing328 wing328 added this to the v2.3.0 milestone Oct 16, 2017
ehyche
ehyche suggested changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@ehyche ehyche left a comment

Choose a reason for hiding this comment

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

@DawidvanGraan : Overall changes look good. I made one comment regarding why .dataDecodingStrategy was being changed since this change was about date decoding, not data decoding.

However, from your comment above that your date format from the server is "YYYY-MM-DD'T'HH:mm:ss.sssZ". The format you describe is ISO8601 exactly:

https://xml2rfc.tools.ietf.org/public/rfc/html/rfc3339.html#anchor14

So I was wondering why setting JSONDecoder.dateDecodingStrategy = .iso8601 did not work for you, and why a customer decoder was needed in your case. I think the change is good and useful, but I didn't understand why it was needed for the date format you describe above.

modules/swagger-codegen/src/main/resources/swift4/CodableHelper.mustache
open class func decode<T>(_ type: T.Type, from data: Data) -> (decodableObj: T?, error: Error?) where T : Decodable {
var returnedDecodable: T? = nil
var returnedError: Error? = nil

let decoder = JSONDecoder()
decoder.dataDecodingStrategy = .base64
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

@DawidvanGraan
Copy link
Contributor Author

@ehyche Currently using .iso8601 is not working for us. I agree that our dateformat is ISO8601, need to look deeper into how the date is decoded and see why it's falling.

Will update the PR and set decoder.dataDecodingStrategy = .base64 correctly. Sorry about this.

@DawidvanGraan
Update CodableHelper.swift
5d24910 
Change to correct format: decoder.dataDecodingStrategy = .base64
@DawidvanGraan
Update CodableHelper.mustache
346e348 
Add `decoder.dataDecodingStrategy = .base64` back
@DawidvanGraan
Update CodableHelper.swift
e18d69a 
Fix `decoder.dataDecodingStrategy = .base64`
@DawidvanGraan
Update CodableHelper.swift
db5af98 
Fix `decoder.dataDecodingStrategy = .base64`
@ehyche
Copy link
Contributor

ehyche commented Nov 13, 2017

@DawidvanGraan : thanks for the update. Looks good to me.

@wing328 : should be good to merge.

@wing328 wing328 merged commit 5af788c into swagger-api:master Nov 13, 2017
@wing328 wing328 mentioned this pull request Nov 13, 2017
Merged
4 tasks
jimschubert added a commit to jimschubert/swagger-codegen that referenced this pull request Nov 14, 2017
@jimschubert
Merge branch 'master' into csharp-enum-cleanup
a0d5079 
* master: (101 commits)
  [Swift4] Allow for custom dateformatter to be used (swagger-api#6672)
  [haskell-http-client] fix bug when generating models-only (swagger-api#6931)
  fix typo: crediential => credential
  minor typo fix
  [csharp] fix enum serialization of first value (swagger-api#6873)
  [PHP] Improve docs and README (swagger-api#6935)
  Binary mode for file deserialization in python (swagger-api#6936)
  add python tornado test to travis
  [Python/tornado] add integration tests and fix bugs (swagger-api#6925)
  Fix PHP passes response body to ApiException (swagger-api#6923)
  [TypeScript][Node] Resolve TS2532 error (swagger-api#6932)
  skip "all" shell script
  minor formatting change
  Fixes Issue swagger-api#6841, Map for accessing additionalProperties is generated. (swagger-api#6886)
  add tsloughter as owner erlang
  WIP: initial commit for Erlang client generator (swagger-api#6502)
  add back php client test
  Switch Travis image from MacOS to Linux (swagger-api#6937)
  add link to ebook
  [Scala] Default case class Option types to None for non-required fields (swagger-api#6790)
  ...
@TakenakaSimon
Copy link

https://bugs.swift.org/browse/SR-5823
Looks like this bug is why iso8601 doesn't work.

Thanks! I'll be using this PR as well.

@wing328
Copy link
Contributor

wing328 commented Nov 17, 2017

@TakenakaSimon you may want to use the SNAPSHOT version of the latest master as mentioned in https://github.com/swagger-api/swagger-codegen#compatibility

fl034
fl034 reviewed Feb 26, 2018
View reviewed changes
modules/swagger-codegen/src/main/resources/swift4/CodableHelper.mustache
if let df = self.dateformatter {
decoder.dateDecodingStrategy = .formatted(df)
} else {
decoder.dataDecodingStrategy = .base64
Copy link

@fl034 fl034 Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@DawidvanGraan Why is this inside the else scope?
Shouldn't the dataDecodingStrategy be applied, regardless of the custom date formatter?
Could be a mistake because date and data looks pretty much the same...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fl034 fl034 fl034 left review comments

@ehyche ehyche ehyche requested changes

Assignees
No one assigned
Labels
Client: Swift Enhancement: General
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@DawidvanGraan @wing328 @ehyche @TakenakaSimon @fl034