Implement Date.description in Swift #179
Conversation
|
cc @compnerd This should fix your Windows issue hopefully |
FranzBusch
changed the title
|
@swift-ci please test |
|
Oddly enough, it does worse: |
FranzBusch
force-pushed
the
fb-date-description
branch
from
d1067db
to
0ca20c1
Compare
|
@swift-ci please test |
| @@ -54,3 +54,12 @@ extension String { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| extension DefaultStringInterpolation { | |||
There was a problem hiding this comment.
I am not really happy with this. I think we can do better here. Are we planning to add the String(format) init in this package?
There was a problem hiding this comment.
Ultimately we will have access to all the interpolation-based formatters, so this seems like an appropriate approach.
|
With the latest changes I think we only have a problem on |
|
That would have to be a lot of leap days to end up adding up to 12 months. What's the numerical value of |
It is |
|
I got a bit further in investigating this the output of our Swift calculation is correct and aligns with |
|
As far as I know |
|
It may be worth it to verify that |
NOTICE.txt
Outdated
| * LICENSE (MIT): | ||
| * https://git.musl-libc.org/cgit/musl/tree/COPYRIGHT | ||
| * HOMEPAGE: | ||
| * https://musl.libc.org |
There was a problem hiding this comment.
It doesn't seem standard to have a NOTICE.txt file in our other Swift repos. We have a license, which is reviewed for correctness.
There was a problem hiding this comment.
We do have a NOTICE.txt file in a bunch of repos for when we use other work or have to attribute it. Some examples:
| @@ -0,0 +1,292 @@ | |||
| //===----------------------------------------------------------------------===// | |||
| // | |||
| // This source file is part of the Swift Collections open source project | |||
There was a problem hiding this comment.
You are right. I copied the header from another file and it turns out that a bunch of files in here have the wrong copyright header.
|
As I chat with @FranzBusch I realized that this would be a huge behavioral change for Darwin users who don't use Gregorian calendar -- previously we use the calendar of their current locale, but now we'd be using Gregorian unconditionally. I personally don't think it's a good idea, but I'm curious to hear what others think. Also we took a look into CFDateFormatter's implementation, and it did look dates before Oct 15, 1582 are formatted with Julian calendar. I think it's fine to change the description for these dates to be in Gregorian if we can just clarify this in comments. |
Just to put some real outputs from the current behaviour of Darwin here. I ran this code with a bunch of different calendars. These are the outputs: @parkera @itingliu What do you all think we should do here? Should we back out the changes for Darwin and just do this on Linux and Windows? |
|
I think the main risk to changing the behavior would be if some app:
If someone was relying on programmatically parsing the value, then they would have likely failed anyway when the user had a non-gregorian calendar set. It does also regress the behavior for a developer debugging their app using a non-gregorian calendar, in that the dates they see in their app will not match the ones they get when |
I don't worry too much about breaking app UI. But this second point is what I was concerned about. It would be a sad regression in developer experience. Would it be possible to ask lldb to format it differently themselves since they're already doing so for other types? |
|
That's possible, and this is actually one of the best types to do that for since it is a very simple memory layout. I'm not sure if lldb has the logic to format dates for other calendars, though, and I doubt they would also be able to read the AppleLocale default to know what the right setting is... |
|
I leave this up to you two here, but if developer experience is really the only thing we are concerned about here than I am sure they can understand if they have to use a I am happy to change the PR to add back the conditional for Darwin here if that makes it easier. It will just mean we have differing behaviour between the platforms. @parkera @itingliu your call :) |
| assert((0...31).contains(date.day)) | ||
| assert((0..<24).contains(date.hours)) | ||
| assert((0..<60).contains(date.minutes)) | ||
| assert((0..<61).contains(date.seconds)) |
There was a problem hiding this comment.
Why are some those ClosedRange while the others Ranges?
There was a problem hiding this comment.
This code is coped over from swift-certificates but I assume this was chosen for readability otherwise we would have to use values such as 23 or 30 where it might be confusing. After reading these asserts I think the author chose the right trade off here.
|
@swift-ci please test |
There was a problem hiding this comment.
This Linux build failure looks legitimate:
[587/689] Compiling FoundationEssentials TimeCalculations.swift
/build/swift-foundation/Sources/FoundationEssentials/TimeCalculations.swift:151:21: error: type 'Int64' has no member 'daysInMonth'
while Int64.daysInMonth(months) <= remdays {
~~~~~ ^~~~~~~~~~~
bb79eab
to
d7ac383
Compare
|
@compnerd Could you test the latest commit of my branch on Windows? |
|
@swift-ci please test |
|
@FranzBusch sorry for the delay, I was travelling. This doesn't seem to address all the problems: I had to apply the following patch: |
|
@compnerd Interesting. Not really sure where this difference is coming from since the Swift code is fully platform independent now. I sadly, don't have a Windows machine where I can reproduce this right now. |
|
See also #252 |
FranzBusch
force-pushed
the
fb-date-description
branch
from
d7ac383
to
05da61e
Compare
|
@swift-ci please test |
|
We're getting closer to being able to just use Foundation's own date/time calculations for this. |
Great to hear. Maybe we can re-use this code then for the ICU-free time calculations. |
|
@FranzBusch What do you think about refactoring this on top of the newly landed Gregorian calendar and ISO8601 formatting changes? |
# Motivation The current implementation of `Date.description` is depending on the what platform we are running on. On Darwin it used the CFFoundation backed implementation and on Linux/Windows it used libc APIs to do the date calculations. This caused build issues on Windows and the libc APIs are not 32bit clean. # Changes This PR changes the implementation to use the `FoundationEssentials` gregorian calendar and ISO 8601 formatter. # Result Swift only implementation that works cross platform for `Date.description` which also should perform better than any libc API calls.
FranzBusch
force-pushed
the
fb-date-description
branch
from
05da61e
to
a4c2a3d
Compare
|
@parkera I updated the code to use the new
I don't see an option to configure the time zone formatting to align with the current formatting of
What would you like to do? |
|
According to spec
But currently I don't think we distinguish between UTC and other GMT+0 timezones. These timezones would compare equal to each other 17> TimeZone(identifier: "GMT+0") == TimeZone(identifier: "UTC")
$R6: Bool = trueThis means that we cannot decide whether to use "Z" or use "+0000" by the time zone instance. And since we cannot tell those apart, I'm inclined to just expose the option directly publicly. But in the short run I think it's ok to expose an internal setting for |
|
I agree with @itingliu -- we can add an internal-only option to the format style to prefer |
|
This line would just need an additional |
Motivation
The current implementation of
Date.descriptionis depending on the what platform we are running on. On Darwin it used the CFFoundation backed implementation and on Linux/Windows it used libc APIs to do the date calculations. This caused build issues on Windows and the libc APIs are not 32bit clean.Changes
This PR changes the implementation by using the methods implemented in swift-certificates that provide timestamp to UTC time calculations. These were added in swift-certificates for the exact same reason. Furthermore, it removes the conditional usage of CFFoundation since the output is the same.
# Result
Swift only implementation that works cross platform for
Date.descriptionwhich also should perform better than any libc API calls.