Date fixes #143

Merged
merged 27 commits into from Mar 28, 2013

Projects

None yet

2 participants

@dannygreg
Contributor

Turns out we have been completely ignoring the timezone offset, instead favouring to always display the date as it would have been in the current timezone as opposed to where the event took place.

This isn't how git log outputs information (or github.com), so we should take it into account when creating NSDate objects.

@joshaber joshaber was assigned Mar 27, 2013
@joshaber
Member

Could you add roughly a bazillion unit tests for this?

@joshaber joshaber commented on an outdated diff Mar 27, 2013
Classes/Categories/NSDate+GTTimeAdditions.m
+// Created by Danny Greg on 27/03/2013.
+// Copyright (c) 2013 GitHub, Inc. All rights reserved.
+//
+
+#import "NSDate+GTTimeAdditions.h"
+
+@implementation NSDate (GTTimeAdditions)
+
++ (NSDate *)gt_dateFromGitTime:(git_time)time {
+ NSInteger seconds = time.time;
+ seconds += time.offset * 60;
+ return [NSDate dateWithTimeIntervalSince1970:seconds];
+}
+
+- (git_time)gt_gitTime {
+ return (git_time){.offset = self.gt_gitTimeOffset, .time = (git_time_t)[self timeIntervalSince1970]};
@joshaber
joshaber Mar 27, 2013 Member

Style: single space padding inside the curly braces.

@joshaber
joshaber Mar 27, 2013 Member

Style: dot syntax for the timeIntervalSince1970 message.

@joshaber joshaber commented on an outdated diff Mar 27, 2013
Classes/Categories/NSDate+GTTimeAdditions.m
+#import "NSDate+GTTimeAdditions.h"
+
+@implementation NSDate (GTTimeAdditions)
+
++ (NSDate *)gt_dateFromGitTime:(git_time)time {
+ NSInteger seconds = time.time;
+ seconds += time.offset * 60;
+ return [NSDate dateWithTimeIntervalSince1970:seconds];
+}
+
+- (git_time)gt_gitTime {
+ return (git_time){.offset = self.gt_gitTimeOffset, .time = (git_time_t)[self timeIntervalSince1970]};
+}
+
+- (int)gt_gitTimeOffset {
+ NSInteger timezoneOffset = [NSTimeZone.defaultTimeZone secondsFromGMT];
@joshaber
joshaber Mar 27, 2013 Member

Style: can secondsFromGMT be dot syntaxed?

@joshaber joshaber commented on an outdated diff Mar 27, 2013
Classes/GTCommit.m
@@ -120,8 +121,8 @@ - (NSString *)messageSummary {
}
- (NSDate *)commitDate {
- time_t t = git_commit_time(self.git_commit);
- return [NSDate dateWithTimeIntervalSince1970:t];
+ git_time time = (git_time){.time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit)};
@joshaber
joshaber Mar 27, 2013 Member

Style: single space padding inside the curly braces.

@joshaber joshaber commented on an outdated diff Mar 27, 2013
Classes/GTSignature.m
@@ -61,8 +62,7 @@ - (id)initWithSignature:(git_signature *)theSignature {
- (id)initWithName:(NSString *)theName email:(NSString *)theEmail time:(NSDate *)theTime {
if((self = [super init])) {
- git_signature_new(&git_signature, [theName UTF8String], [theEmail UTF8String], (git_time_t) [theTime timeIntervalSince1970], 0);
- // todo: figure out offset for NSDate
+ git_signature_new(&git_signature, [theName UTF8String], [theEmail UTF8String], (git_time_t) [theTime timeIntervalSince1970], theTime.gt_gitTimeOffset);
@joshaber
joshaber Mar 27, 2013 Member

Style: dot syntax all that shiz, and remove the space after the cast.

@joshaber
Member

Mostly just style 🐛

Do you think we need to export NSDate+GTTimeAdditions? Or should callers never need to bother with that manually.

@dannygreg
Contributor

Could you add roughly a bazillion unit tests for this?

Absolutely. Should have done that in the first place.

Do you think we need to export NSDate+GTTimeAdditions? Or should callers never need to bother with that manually.

Callers should only be dealing with NSDate, if someone is having to use git_time we've failed as a wrapper.

@joshaber
Member

Callers should only be dealing with NSDate, if someone is having to use git_time we've failed as a wrapper.

👍 That's what I was hoping.

@dannygreg
Contributor

I've been doing some more reading around since I started implementing the unit tests. I think my approach here is wrong.

An NSDate simply refers to a point in time and has no concept of timezones. It merely represents a number of seconds since a reference date. A time zone is only meant to come into play when rendering a representation of the date with an NSDateFormatter.

With that in mind, I don't think compensating for the offset here makes much sense. Instead we should support both creation of an NSTimeZone object which represents the offset as well as an NSDate to represent the timestamp.

Will mull over an API and update this pull.

@dannygreg
Contributor

Take a look at that lot @joshaber, see if it tickles your fancy.

@joshaber joshaber and 1 other commented on an outdated diff Mar 28, 2013
Classes/Categories/NSDate+GTTimeAdditions.h
+// ObjectiveGitFramework
+//
+// Created by Danny Greg on 27/03/2013.
+// Copyright (c) 2013 GitHub, Inc. All rights reserved.
+//
+
+#import <Foundation/Foundation.h>
+
+#import "git2.h"
+
+@interface NSDate (GTTimeAdditions)
+
+// Creates a new `NSDate` from the provided `git_time`.
+//
+// time - The `git_time` to base the returned date on.
+// timeZone - The timezone used by the time passed in.
@joshaber
joshaber Mar 28, 2013 Member

Can this be NULL?

@dannygreg
dannygreg Mar 28, 2013 Contributor

Totes. Will document it as optional.

@joshaber joshaber commented on an outdated diff Mar 28, 2013
Classes/GTCommit.m
@@ -120,8 +121,15 @@ - (NSString *)messageSummary {
}
- (NSDate *)commitDate {
- time_t t = git_commit_time(self.git_commit);
- return [NSDate dateWithTimeIntervalSince1970:t];
+ git_time time = (git_time){.time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit)};
+ return [NSDate gt_dateFromGitTime:time timeZone:NULL];
+}
+
+- (NSTimeZone *)commitTimeZone {
+ NSTimeZone *timeZone = nil;
+ git_time time = (git_time){.time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit)};
@joshaber
joshaber Mar 28, 2013 Member

Style: single space padding inside the curly braces.

@joshaber joshaber commented on an outdated diff Mar 28, 2013
Classes/GTSignature.h
@@ -37,6 +37,7 @@
@property (nonatomic, copy) NSString *name;
@property (nonatomic, copy) NSString *email;
@property (nonatomic, strong) NSDate *time;
+@property (nonatomic, readonly) NSTimeZone *timeZone;
@joshaber
joshaber Mar 28, 2013 Member

Even though it's readonly, should still include the ownership semantics.

@joshaber joshaber and 1 other commented on an outdated diff Mar 28, 2013
ObjectiveGitTests/GTTimeAdditionsSpec.m
@@ -0,0 +1,53 @@
+//
+// GTTimeAdditionsSpec.m
+// ObjectiveGitFramework
+//
+// Created by Danny Greg on 27/03/2013.
+// Copyright (c) 2013 GitHub, Inc. All rights reserved.
+//
+
+#import "NSDate+GTTimeAdditions.h"
+
+SpecBegin(GTTimeAdditions)
+
+describe(@"Convertion between git_time and NSDate", ^{
@joshaber
joshaber Mar 28, 2013 Member

"Convertion" totes isn't a word.

@dannygreg
dannygreg Mar 28, 2013 Contributor

I am nothing without spellcheck.

@joshaber joshaber commented on an outdated diff Mar 28, 2013
Classes/GTCommit.m
@@ -120,8 +121,15 @@ - (NSString *)messageSummary {
}
- (NSDate *)commitDate {
- time_t t = git_commit_time(self.git_commit);
- return [NSDate dateWithTimeIntervalSince1970:t];
+ git_time time = (git_time){.time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit)};
+ return [NSDate gt_dateFromGitTime:time timeZone:NULL];
+}
+
+- (NSTimeZone *)commitTimeZone {
+ NSTimeZone *timeZone = nil;
+ git_time time = (git_time){.time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit)};
+ [NSDate gt_dateFromGitTime:time timeZone:&timeZone];
@joshaber
joshaber Mar 28, 2013 Member

This API seems awkward. Here we're calling +[NSDate gt_dateFromGitTime:timeZone:] and ignoring its return value. And a few lines up, we're calling it with timeZone:NULL. That seems like it's asking for different APIs.

@dannygreg
Contributor

No idea if you finished your review, but I've patched up your points. Also merged in from master as there was a project file conflict.

@joshaber joshaber and 1 other commented on an outdated diff Mar 28, 2013
Classes/GTCommit.m
@@ -120,8 +121,15 @@ - (NSString *)messageSummary {
}
- (NSDate *)commitDate {
- time_t t = git_commit_time(self.git_commit);
- return [NSDate dateWithTimeIntervalSince1970:t];
+ git_time time = (git_time){ .time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit) };
+ return [NSDate gt_dateFromGitTime:time timeZone:NULL];
+}
+
+- (NSTimeZone *)commitTimeZone {
+ NSTimeZone *timeZone = nil;
+ git_time time = (git_time){ .time = git_commit_time(self.git_commit), .offset = git_commit_time_offset(self.git_commit) };
+ [NSDate gt_dateFromGitTime:time timeZone:&timeZone];
@joshaber
joshaber Mar 28, 2013 Member

(Reposting because it was collapsed.)

This API seems awkward. Here we're calling +[NSDate gt_dateFromGitTime:timeZone:] and ignoring its return value. And a few lines up, we're calling it with timeZone:NULL. That seems like it's asking for different APIs.

@dannygreg
dannygreg Mar 28, 2013 Contributor

My glass castle just came crashing down around me.

I think you're right.

@joshaber
Member

🛀

@dannygreg
Contributor

🚿

Hows about that one @joshaber?

@joshaber joshaber commented on an outdated diff Mar 28, 2013
Classes/Categories/NSDate+GTTimeAdditions.h
+// NSDate+GTTimeAdditions.h
+// ObjectiveGitFramework
+//
+// Created by Danny Greg on 27/03/2013.
+// Copyright (c) 2013 GitHub, Inc. All rights reserved.
+//
+
+#import <Foundation/Foundation.h>
+
+#import "git2.h"
+
+@interface NSDate (GTTimeAdditions)
+
+// Creates a new `NSDate` from the provided `git_time`.
+//
+// time - The `git_time` to base the returned date on.
@joshaber
joshaber Mar 28, 2013 Member

Style: unnecessary space before the -

@joshaber
Member

💧

Just a one 👍

@dannygreg
Contributor

💥

@joshaber
Member

💥 🍸

@joshaber joshaber merged commit 51df316 into master Mar 28, 2013

1 check passed

default Build #335096 succeeded in 8s
Details
@joshaber joshaber deleted the date-fixes branch Mar 28, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment