Skip to content

Support different options in ISO8601DateTranscoder #389

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

Closed
bartoszirzyk opened this issue Nov 24, 2023 · 7 comments · Fixed by apple/swift-openapi-runtime#94
Closed

Support different options in ISO8601DateTranscoder #389

bartoszirzyk opened this issue Nov 24, 2023 · 7 comments · Fixed by apple/swift-openapi-runtime#94
Assignees
Labels
kind/enhancement Improvements to existing feature. status/needs-design Needs further discussion and a concrete proposal.
Milestone

Comments

@bartoszirzyk
Copy link

Hi,
for now DateTranscoder exposes only one type var iso8601. This is great but I think it might be even better ;)
Would be great to extend this declaration to be able to setup ISO8601DateFormatter with options that it provides https://developer.apple.com/documentation/foundation/iso8601dateformatter/options

e. g. I've got a date from backend "2023-09-16T14:27:13.554Z" and it cannot be parsed by default transcoder.
Using formatter.formatOptions = [.withInternetDateTime, .withFractionalSeconds] on ISO8601DateFormatter I can easily do that.
I can create a Custom DateTranscoder but I feel bad about that since it will be an ISO8601DateTranscoder as well but with options. This might be also very useful for a community to use

@czechboy0
Copy link
Contributor

Yeah this has come up a few times. Specifically the fractional seconds seems like the most common reason to customize it.

Maybe we should provide a second built-in transcoder, ISO8601DateTranscoderWithFractionalSeconds and a var iso8601WithFractionalSeconds, to make it easy to switch?

Ideas welcome, we want to balance ergonomics with maintainability.

@czechboy0 czechboy0 added status/needs-design Needs further discussion and a concrete proposal. kind/enhancement Improvements to existing feature. labels Nov 24, 2023
@czechboy0 czechboy0 added this to the Post-1.0 milestone Nov 24, 2023
@bartoszirzyk
Copy link
Author

bartoszirzyk commented Nov 24, 2023

Hmm there is another point to keep in mind. Documentation says changing options/timeZone:

Please note that there can be a significant performance cost when resetting these properties.  
Resetting each property can result in regenerating the entire CFDateFormatterRef, which can be very expensive.

Now ISO8601DateTranscoder initialises new formatter each encode/decode. I believe that making same thing with changing options may fire that regenerating entire CFDateFormatterRef.

Whenever I want to save an modified formatter in my transcoder to avoid multiple creation there will be a warning about Sendable (My transcoder which is Sendable has non-Sendable ISO8601DateFormatter).

@czechboy0
Copy link
Contributor

If you're providing the locking around ISO8601DateFormatter, then you'll need to mark your transcoder as @unchecked Sendable, that way you're promising to the compiler that you're doing synchronization manually.

@fermentfan
Copy link

Ouf I just met this unfortunate behaviour... Is there a time plan for making this configurable? We currently write fractional seconds to the database and it'd be very cumbersome to change this, just because of our iOS stack 🥲

Coming from the Javascript world no one would ever ask if you should throw an error for something like that haha.

@czechboy0
Copy link
Contributor

czechboy0 commented Jan 14, 2024

@fermentfan Can you be more specific? You can provide any date formatter you like, provide it to the Configuration struct.

If you like to use Foundation's DateFormatter, make sure to configure it with fractional seconds, if you need that.

Swift OpenAPI Generator itself isn't implementing the formatting logic itself, it fully delegates to Foundation or your custom transcoder.

@czechboy0
Copy link
Contributor

Ok is this what you had in mind, @fermentfan? apple/swift-openapi-runtime#94

@czechboy0 czechboy0 self-assigned this Jan 14, 2024
@fermentfan
Copy link

Damn thank you for the quick support @czechboy0! I definitely misunderstood the issue here and so I thought it is not possible at all to configure the DateTranscoder at the moment. With your remark I noticed what the point of the thread really is and what the proposed solution is - which I totally like. I now at least locally copied around the internal transcoder and fitted it to my needs, but I am looking forward to reducing copy pasted code :) I like the approach in your PR (at least what I understand as I am super new to Swift).

Btw the repository's test suite helped me immensely in understanding what to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature. status/needs-design Needs further discussion and a concrete proposal.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants