Skip to content
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

Specify locale to ensure ISO 8601 date formatters work as expected #81

Merged
merged 4 commits into from
Apr 29, 2016

Conversation

brockboland
Copy link
Contributor

@chillpop
Copy link
Contributor

LGTM, do you think it's worth adding a test for this?

@brockboland
Copy link
Contributor Author

Probably, but don't have time to do so right now. Feel like taking a crack at it?

@chillpop
Copy link
Contributor

sure

@brockboland
Copy link
Contributor Author

@chillpop's test has been merged. Anyone else want to have a look, @vokal/ios-developers?

@brockboland
Copy link
Contributor Author

Oh wait, one more thing: I'll do a bugfix release for this.

@brockboland
Copy link
Contributor Author

I take that back: updating the version number in the podspec means cascading updates to the pods directory in the sample projects, so I'll open another PR for that after this one is merged.

@@ -57,6 +57,7 @@ + (NSDateFormatter *)vok_defaultDateFormatter
DefaultDateFormatter = [NSDateFormatter new];
DefaultDateFormatter.dateFormat = @"yyyy-MM-dd'T'HH:mm:ss.SSSSSS'Z'";
DefaultDateFormatter.timeZone = [NSTimeZone timeZoneForSecondsFromGMT:0];
DefaultDateFormatter.locale = [NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make a constant out of this string literal in the .h file? That way it can be shared here and in tests?

@seanwolter
Copy link
Contributor

seanwolter commented Apr 28, 2016

Neato.
Lol pasted wrong thing. Y Apple no provide a constant for "en_US_POSIX"? 🤷

@bryanluby
Copy link
Contributor

@brockboland This would make a great show & tell at the next NSCoder

@brockboland
Copy link
Contributor Author

@bryanluby good points

@brockboland
Copy link
Contributor Author

Updated

@bryanluby
Copy link
Contributor

LGTM

VOKDefaultDateFormatterLocaleIdentifier);
XCTAssertEqualObjects([VOKManagedObjectMap vok_dateFormatterWithoutMicroseconds].locale.localeIdentifier,
VOKDefaultDateFormatterLocaleIdentifier);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My instinct is that it'd be better to test the output of the date formatter when applied to various date strings with various current locales, but I'm not sure offhand how to set the current locale programmatically for testing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, you can only set the locale by passing in process arguments or choosing a locale in the Xcode scheme:
https://developer.apple.com/library/ios/documentation/MacOSX/Conceptual/BPInternational/TestingYourInternationalApp/TestingYourInternationalApp.html
http://useyourloaf.com/blog/using-launch-arguments-to-test-localizations/
Which makes testing that way a bit of a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless we move this test to a new test target and add a scheme specifically for it. I haven't done any testing to figure out what region or locale we could use for this that would change the calendar. You can switch it in Settings > General > Language & Region > Calendar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like more trouble than it's worth, then.

@brockboland
Copy link
Contributor Author

@vokal-isaac good to pull, then?

@vokal-isaac
Copy link
Contributor

(:goat:,)

@brockboland
Copy link
Contributor Author

thanks!

@brockboland brockboland merged commit 5dc3017 into vokal:master Apr 29, 2016
@brockboland brockboland deleted the date_formatter branch April 29, 2016 16:55
@brockboland brockboland mentioned this pull request Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants