Add support for time zones #1
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snowy1803
requested changes
Aug 3, 2022
| self.init(base: base, timeZone: nil) | ||
| } | ||
|
|
||
| init(base: Date = Date(), timeZone: TimeZone?) { |
There was a problem hiding this comment.
you could just put a default argument value of nil for timeZone instead of creating another initialiser
| } | ||
|
|
||
| init(base: Date = Date(), timeZone: TimeZone?) { | ||
| var GregorianCalendar: Calendar = Calendar.gregorian |
There was a problem hiding this comment.
Use the Swift naming conventions: lowerCamelCase for variables
| "Chamerops": "Chamaerops_humilis", | ||
| "Épine vinette": "%C3%89pine-vinette", | ||
| "Verge d'or": "Solidago" | ||
| "Belle de nuit": "Mirabilis_jalapa", "Amaryllis": "Amaryllis_(plante)", "Erable sucré": "%C3%89rable_%C3%A0_sucre", "Perce Neige": "Perce-neige", "Laurier thym": "Viorne_tin", "Thimèle": "Daphn%C3%A9_garou", "Bâton d'or": "Girofl%C3%A9e_des_murailles", "Chamerops": "Chamaerops_humilis", "Épine vinette": "%C3%89pine-vinette", "Verge d'or": "Solidago" |
There was a problem hiding this comment.
avoid reformatting code, it's more readable line per line
Snowy1803
changed the title
…nCalendarCore when I created a pull request.
|
Requested changes have been made. Have run the unit tests just to make sure everything still works. |
Snowy1803
reviewed
Aug 4, 2022
| self.init(romanYear: romanYear, variant: variant, treatSansculottidesAsAMonth: false) | ||
| } | ||
|
|
||
| public init(romanYear: Bool, variant: Variant, treatSansculottidesAsAMonth: Bool) { |
There was a problem hiding this comment.
Could use a default value of false for treatSansculottidesAsAMonth instead of duplicating here too
|
It could also be useful to add unit tests for your additions; it's not necessary for treatSansculottidesAsAMonth, it's pretty straightforward, but the time zone support could be tested |
|
Thanks. I forgot about that one.
Aaron Adelman
… ב-4 באוג׳ 2022, בשעה 18:38, Emil Pedersen ***@***.***> כתב/ה:
@Snowy1803 commented on this pull request.
In Sources/FrenchRepublicanCalendarCore/FrenchRepublicanDateOptions.swift <#1 (comment)>:
> public init(romanYear: Bool, variant: Variant) {
+ self.init(romanYear: romanYear, variant: variant, treatSansculottidesAsAMonth: false)
+ }
+
+ public init(romanYear: Bool, variant: Variant, treatSansculottidesAsAMonth: Bool) {
Could use a default value of false for treatSansculottidesAsAMonth instead of duplicating here too
—
Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCTW3FPA6OJK5JIHBKFDXLVXPPXRANCNFSM55ORMWIQ>.
You are receiving this because you authored the thread.
|
|
Will see what I can do about that.
Aaron Adelman
… ב-4 באוג׳ 2022, בשעה 18:41, Emil Pedersen ***@***.***> כתב/ה:
It could also be useful to add unit tests for your additions; it's not necessary for treatSansculottidesAsAMonth, it's pretty straightforward, but the time zone support could be tested
—
Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCTW3ENFREM3QGZV52LXI3VXPQCRANCNFSM55ORMWIQ>.
You are receiving this because you authored the thread.
|
|
Good call asking for time zone support unit tests. I’m getting some odd results for Anchorage and Honolulu. Looks like there’s some serious debugging to do to figure out what went wrong.
Aaron Adelman
… ב-4 באוג׳ 2022, בשעה 18:44, ד״ר אַהֲרֹן שְׁלֹמֹה אֵדֶלְמָן PhD ***@***.***> כתב/ה:
Will see what I can do about that.
Aaron Adelman
> ב-4 באוג׳ 2022, בשעה 18:41, Emil Pedersen ***@***.*** ***@***.***>> כתב/ה:
>
>
> It could also be useful to add unit tests for your additions; it's not necessary for treatSansculottidesAsAMonth, it's pretty straightforward, but the time zone support could be tested
>
> —
> Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCTW3ENFREM3QGZV52LXI3VXPQCRANCNFSM55ORMWIQ>.
> You are receiving this because you authored the thread.
>
|
|
Sorry for this taking so long, but other things happening over here have made it not optimal for debugging. After a lot making it very explicit what is being tested, the tests finally all pass. This includes testing in the user’s time zone, 10 different time zones from across the planet, testing for 1970-01-01, and testing for 2022-08-08 (the date that particular subtest was written).
Enjoy.
Aaron Adelman
ד״ר אַהֲרֹן שְׁלֹמֹה אֵדֶלְמָן • Aaron Solomon Adelman, PhD
מתכנת • Programmer
***@***.***
http://www.linkedin.com/in/aaronsolomonadelman
… ב-4 באוג׳ 2022, בשעה 22:53, ד״ר אַהֲרֹן שְׁלֹמֹה אֵדֶלְמָן PhD ***@***.***> כתב/ה:
Good call asking for time zone support unit tests. I’m getting some odd results for Anchorage and Honolulu. Looks like there’s some serious debugging to do to figure out what went wrong.
Aaron Adelman
> ב-4 באוג׳ 2022, בשעה 18:44, ד״ר אַהֲרֹן שְׁלֹמֹה אֵדֶלְמָן PhD ***@***.*** ***@***.***>> כתב/ה:
>
> Will see what I can do about that.
>
> Aaron Adelman
>
>> ב-4 באוג׳ 2022, בשעה 18:41, Emil Pedersen ***@***.*** ***@***.***>> כתב/ה:
>>
>>
>> It could also be useful to add unit tests for your additions; it's not necessary for treatSansculottidesAsAMonth, it's pretty straightforward, but the time zone support could be tested
>>
>> —
>> Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHCTW3ENFREM3QGZV52LXI3VXPQCRANCNFSM55ORMWIQ>.
>> You are receiving this because you authored the thread.
>>
>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In incorporating FrenchRepublicanCalendarCore into one of my projects, I realized that it did not support time zones, so I fixed it. I also made it possible to treat Sansculottides as a 13th month in formatted dates, and I was very careful to make sure that the original API was still valid. Enjoy.