-
Notifications
You must be signed in to change notification settings - Fork 381
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
Overloaded -timePassed method with language agnostic implementation #486
Conversation
commit 0f42588 Author: Jaja Yting <jaltying@gmail.com> Date: Fri Aug 24 13:23:26 2018 +0800 Added tests for the new -timePassed method commit abe8423 Author: Jaja Yting <jaltying@gmail.com> Date: Fri Aug 24 13:10:19 2018 +0800 Conformance to Equatable Protocol commit 67d77fa Author: Jaja Yting <jaltying@gmail.com> Date: Fri Aug 24 13:05:52 2018 +0800 Declared TimePassed enum commit 0af7cc7 Author: Jaja Yting <jaltying@gmail.com> Date: Fri Aug 24 13:04:53 2018 +0800 Added new file for TimePassed enum commit 637997a Author: Jaja Yting <jaltying@gmail.com> Date: Fri Aug 24 12:54:05 2018 +0800 Renamed TimePassedUnit > TimePassed and declared it outside the extension commit c40ac53 Author: Jaja Yting <jaltying@gmail.com> Date: Thu Aug 23 21:35:22 2018 +0800 Revert "Added a file to declare the TimePassedUnit enum" This reverts commit 8228178. commit 8228178 Author: Jaja Yting <jaltying@gmail.com> Date: Thu Aug 23 21:32:44 2018 +0800 Added a file to declare the TimePassedUnit enum commit bbd503b Author: Jaja Yting <jaltying@gmail.com> Date: Thu Aug 23 21:26:52 2018 +0800 Implemented method to return an enum of time passed commit 8277b9a Author: Jaja Yting <jaja@jajas-macbook-pro.local> Date: Tue Aug 7 16:52:33 2018 +0800 Added "now" to represent time passed less than one second commit 92f188c Author: Jaja Yting <jaja@jajas-macbook-pro.local> Date: Tue Aug 7 16:48:58 2018 +0800 Declared an enum with cases with associated values for time passed
Generated by 🚫 Danger |
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
=========================================
+ Coverage 43.09% 44.2% +1.11%
=========================================
Files 50 51 +1
Lines 2265 2312 +47
=========================================
+ Hits 976 1022 +46
- Misses 1289 1290 +1
Continue to review full report at Codecov.
|
let calendar = Calendar.autoupdatingCurrent | ||
let components = (calendar as NSCalendar).components([.year, .month, .day, .hour, .minute, .second], from: self, to: date, options: []) | ||
|
||
if components.year! >= 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you have to represent something like 1 year and 6 months. IMO it should be both TimePassed.year(1) and TimePassed.month(6). Here it would just round up to 1 year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it would just round up to the largest time unit, because we retained the original behaviour of the first -timePassed
extension method. The original method would return "1 year ago"
, instead of "1 year and 6 months ago"
.
On developers' (consumers') perspective, you're still right on what the -timePassed
method should return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes fair point. I just looked up the impl.
The logic given that impl is fine, but that is something that might be construed as incorrect. I will think about how to deprecate that version and actually fix that impl with a better impl, but this is fine for now.
The overload of the method -timePassed returns an enum of TimePassed. Instances of TimePassed can be
.year(int)
,.month(int)
,.day(int)
,.hour(int)
,.minute(int)
,.second(int)
, andnow
.Original implementation of the -timePassed method returns a string eg.
5 seconds ago
,13 years ago
. It is really convenient for the consumer of this method, if the target build is on english, but it is really cumbersome if the target is non-english builds.Checklist