Expressively initialising calendars#1923
Conversation
| timeZone: TimeZone, | ||
| locale: Locale? = nil, | ||
| firstWeekday: Int? = nil | ||
| ) |
There was a problem hiding this comment.
if we're doing this, should this just be a member-wise initializer that includes all settable variables? If we do that we'd want to include var minimumDaysInFirstWeek: Int too
There was a problem hiding this comment.
Thanks @itingliu for your time and effort, I appreciate it.
In c27294e, I've reframed this initialiser as one for publicly settable properties that affect calendrical computations, rather than as merely an indiscriminate member-wise initialiser.
I do wonder if: 1) that reframing is redundant -- a calendar just is responsible for calendrical computations and; 2) ABI stability prevents the initialiser from changing shape if new properties (with sensible default values) that also affect calendrical computations are added.
For example, let us say that Calendar publicly exposed minimumDaysInFirstWeek after this initialiser was in the ABI. The vast majority of use cases can accept a sensible default value, so the initialiser could take on a new shape that includes minimumDaysInFirstWeek: Int? = nil. This couldn't be done in a way that avoids breaking the ABI. So the initialiser can't maintain its responsibility as an initialiser for publicly settable properties that affect calendrical computations. To this I would suggest that such a property is not foreseeable after the mileage Calendar and NSCalendar has afforded developers all this time. And even if it was foreseeable, I would expect its use case to be so exotic as to make changing the property's default value separate to initialising the Calendar a regrettable but tolerable compromise.
There was a problem hiding this comment.
For example, let us say that Calendar publicly exposed minimumDaysInFirstWeek after this initialiser was in the ABI.
I'm not sure what you meant by it. This minimumDaysInFirstWeek is already public now. And given that we haven't added new variables on Calendar for a while I think the concerns is fairly small
There was a problem hiding this comment.
I was trying to walk through an example of what would happen if a public property was added after the initialiser was in the ABI. I do not think there are any ABI stability concerns impeding this.
I've done some investigation and updated the proposal and implementation again.
There was a problem hiding this comment.
I think there would only be an ABI concern if we add another new settable property in the future, and we want to amend the initializer to take that property. But there are a few things we can do that's still ABI safe
- Don't put the new property in initializers at all. Precedent:
Date.Component's initializer has most date component fields, but it does not havedayOfYear, which was added later - Add another initializer that takes that property.
- Add that new property to this initializer with a default argument.
So yeah I don't think this is a deal breaker.
There was a problem hiding this comment.
- Add that new property to this initializer with a default argument.
I understood this would in fact be an ABI breaking change because the method signature still changes, even if it has a default value that makes it look "as if" it was the same initialiser at the call site?
Thanks for the tip about DateComponents. Do you think I've adequately captured those three points already in the proposal?
There was a problem hiding this comment.
Hm yeah you're right about it. We will need a new availability too.
3b2c05f to
c27294e
Compare
|
|
||
| ```swift | ||
| public init( | ||
| identifier: __shared Identifier, |
There was a problem hiding this comment.
Is there a reason to use a superseded underscored ownership modifier here?
There was a problem hiding this comment.
👋 If by "here" you mean the proposal text, then as I understand it, the proposal should contain definition in code. If by "here" you mean in the code, then I am merely following what the Calendar API uses for its existing initialiser, and I defer to that reason.
Thanks for reading!
There was a problem hiding this comment.
Yeah. I mean both: we don't typically show non-public features in interfaces, and in any case it doesn't seem like the new API requires specifically the superseded ownership modifier.
There was a problem hiding this comment.
I actually can't find any differences performance-wise with __shared or not, so I don't think we strictly need to follow the precedent.
|
In 5cf8c57, explored tera's suggestion for a Fluent style configuration initializer. Keen to hear what people think. |
itingliu
left a comment
There was a problem hiding this comment.
Can you move the proposal file into a separate PR so we can merge that and start the review, if you would also want to do that?
Add a
Calendarinitializer that accepts aTimeZoneand optionalLocaleandfirstWeekdayvalues, allowing developers to expressively and efficiently initialize a fully configured calendar in a single expression.Motivation:
Today, if we want a
Calendarwith non-default time zone, locale, first weekday, or minimum days in first week values, we have to initialize the calendar with its identifier and then set its properties, one after the other:Some observations about this code:
Modifications:
The new initializer is defined as:
The initializer is responsible for accepting a value for every publicly settable property of
Calendarthat affects calendrical computations. Presently, it is practically — but not necessarily — a member-wise initializer.Internally, the initializer sets the values for the properties at once, avoiding the repeated initialization of the underlying calendar storage when setting properties one after the other.
When
timeZone,locale,firstWeekday, orminimumDaysInFirstWeekarenil, the resulting calendar uses the same default values for those corresponding properties asinit(identifier:).Result:
Consider
CalendarTests.swift. For example, instead of writing:…test authors can write:
This allows test authors to write a single expression that statically expresses this calendar object does not change after it is defined (that is, the calendar object's value is constant). It also saves approximately 40 lines of code. It is also marginally more efficient.
Testing:
CalendarTests.swiftincludes a test for the new initializer.