Change DateFormat to use the full ISO8601 date format by default
#45
Conversation
|
@tanner0101 would you mind taking a look at this? |
|
Given that users can already provide any date format as an argument to the Leaf tag, I'm not feeling strongly about what exactly the default should be. However, I read somewhere (I think in the docs, but can't find it right now) that an ISO8601 date format will be used by default, so that documentation should probably be updated. |
|
I think it could make sense to move Vapor 4's Leaf to using 8601 by default. That seems like the better choice. For 3 though, to avoid surprising anyone, we could make this configurable. Perhaps by passing a custom Then we could even provide some static methods for creating an 8601-default tag. |
…er' of github.com:vapor/template-kit into patch-4.
|
@tanner0101 I have made this configurable now; please take another look. |
There was a problem hiding this comment.
DateFormatter is thread safe on new versions of macOS apparently:
In macOS 10.9 and later NSDateFormatter is thread safe so long as you are using the modern behavior in a 64-bit app.
But I'm not sure if this holds true for Linux. The safer option here would be to not share them between threads.
Unfortunately in Vapor 3, a single tag can be shared across threads, so if we store a DateFormatter there directly, it will be used from multiple threads. Storing a closure that creates a DateFormatter could solve this. Static methods of course will be shared across threads, too.
(Side note, in Vapor 4 all services are bound to an event loop. So it would be appropriate there to store the DateFormatter directly).
| private let defaultDateFormatter: DateFormatter | ||
|
|
||
| private static let dateAndTimeFormatter: DateFormatter = { | ||
| let dateFormatter = DateFormatter() |
There was a problem hiding this comment.
DateFormatter is a class and I don't think it's thread safe. This probably needs to be a factory method that produces a new formatter each time.
There was a problem hiding this comment.
I spotted the same issue and checked; it is thread safe: https://developer.apple.com/documentation/foundation/dateformatter#1680059
There was a problem hiding this comment.
https://bugs.swift.org/browse/SR-7745
That documentation references Darwin platforms. I'm not sure it holds true for Linux. Vapor has definitely had issues with DateFormatter and multi-threading in the past. Maybe they are resolved now? Vapor 3 must support Ubuntu 14.04 and Swift 4.1.x though. So even if multi-threading has been fixed recently, we can't really rely on that.
| /// Creates a new `DateFormat` tag renderer. | ||
| /// - parameter defaultDateFormatter: The date formatter to use when the tag invocation | ||
| /// does not specify a date format. | ||
| public init(defaultDateFormatter: DateFormatter) { |
There was a problem hiding this comment.
This also probably needs to be an escaping closure that creates a DateFormatter
|
@tanner0101 thank you for the elaboration! And sorry, I hadn't seen your main review comment. I've updated the PR to use a date formatter factory now. |
|
@tanner0101 could you please take another look at this? |
|
Friendly ping @tanner0101 :-) |
There was a problem hiding this comment.
This LGTM, although I'd like to make sure the tests run on Linux. Can you run swift test --generate-linuxmain here to ensure all of the new ones are running in CI?
It might also be useful to do a pressure test on DateFormat across two threads just to be sure this change won't introduce any seg faults on Linux.
|
Updated the test manifest and added a stress test; please take another look. |
|
Hey @MrMage, you just merged a pull request, have a coin! You now have 30 coins. |
No description provided.