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

[Swift3] add en_US_POSIX locale to date formatters #5602

Merged
merged 1 commit into from
Jul 4, 2017
Merged

[Swift3] add en_US_POSIX locale to date formatters #5602

merged 1 commit into from
Jul 4, 2017

Conversation

julienfouilhe
Copy link
Contributor

@julienfouilhe julienfouilhe commented May 10, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

The same thing has been done for the swift generator, it resolves the same issue, described there: #2159
#5008

The only file edited is this one: modules/swagger-codegen/src/main/resources/swift3/Models.mustache

@wing328
Copy link
Contributor

wing328 commented May 17, 2017

@julienfouilhe thanks for the PR.

cc @Edubits @jaz-ah @jgavris for review.

@jgavris
Copy link
Contributor

jgavris commented May 17, 2017

👍 possible / easy to isolate a unit test in the example suite?

@julienfouilhe
Copy link
Contributor Author

@jgavris Where are they located? I don't see any swift tests.

@wing328
Copy link
Contributor

wing328 commented May 20, 2017

@julienfouilhe you can find the test cases for Swift3 Petstore client under https://github.com/swagger-api/swagger-codegen/tree/master/samples/client/petstore/swift3

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented May 21, 2017

@wing328 @jgavris After giving it some thought I'm not sure how to test this as it depends on the user's device locale. But here is a link to Apple's recommendations: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/DataFormatting/Articles/dfDateFormatting10_4.html

If you're working with fixed-format dates, you should first set the locale of the date formatter to something appropriate for your fixed format. In most cases the best locale to choose is en_US_POSIX, a locale that's specifically designed to yield US English results regardless of both user and system preferences. en_US_POSIX is also invariant in time (if the US, at some point in the future, changes the way it formats dates, en_US will change to reflect the new behavior, but en_US_POSIX will not), and between platforms (en_US_POSIX works the same on iPhone OS as it does on OS X, and as it does on other platforms).

Once you’ve set en_US_POSIX as the locale of the date formatter, you can then set the date format string and the date formatter will behave consistently for all users.

@jgavris
Copy link
Contributor

jgavris commented May 21, 2017

I've had success swizzling +[NSLocale currentLocale] in tests (not in this project though). You could maybe do one or two examples.

@wing328
Copy link
Contributor

wing328 commented Jul 1, 2017

@jgavris @jaz-ah @Edubits shall we add the tests in a separate PR instead and merge this PR into master if the PR looks good and has been tested manually?

@jaz-ah
Copy link
Contributor

jaz-ah commented Jul 3, 2017

i'm ok w/ that @wing328

@jgavris
Copy link
Contributor

jgavris commented Jul 4, 2017

👍 as well, sorry for the delay

@wing328 wing328 merged commit c2b5756 into swagger-api:master Jul 4, 2017
@wing328 wing328 changed the title fix(swift3): add en_US_POSIX locale to date formatters [Swift3] add en_US_POSIX locale to date formatters Jul 4, 2017
dmpliamplias pushed a commit to inventage/swagger-codegen that referenced this pull request Jul 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants