Use Locale.US for Android #238
Conversation
|
Thankss @Mecharyry - can you elaborate on the breaking change? Does it introduce one? |
Sorry I goofed when writing the description. I meant to write that the test class illustrates why parsing an Arabic specific date time breaks vs a US one. I've updated the description. |
|
@stevehobbsdev I'd prefer this solution as well over the other. LGTM 👍 |
| @@ -55,7 +55,7 @@ class LoginApiRequestHandler : ApiRequestHandler { | |||
| override fun onSuccess(credentials: Credentials) { | |||
| val scope = credentials.scope?.split(" ") ?: listOf() | |||
| val sdf = | |||
| SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.getDefault()) | |||
| SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.US) | |||
There was a problem hiding this comment.
Can we use the format yyyy-MM-dd'T'HH:mm:ss.SSS'Z' as that is the one used in Android and iOS.
| @@ -43,7 +43,7 @@ class LoginWithOtpApiRequestHandler: ApiRequestHandler { | |||
| override fun onSuccess(credentials: Credentials) { | |||
| val scope = credentials.scope?.split(" ") ?: listOf() | |||
| val sdf = | |||
| SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.getDefault()) | |||
| SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ssZ", Locale.US) | |||
There was a problem hiding this comment.
Same as above for all the format strings
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
============================================
- Coverage 98.74% 98.67% -0.08%
- Complexity 82 85 +3
============================================
Files 80 85 +5
Lines 1439 1506 +67
Branches 320 331 +11
============================================
+ Hits 1421 1486 +65
- Misses 6 7 +1
- Partials 12 13 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
@poovamraj Sorry for the delay, I've made the requested changes! |
|
@poovamraj @stevehobbsdev Can I get some eyes on this? It would be great to merge this so I can stop maintaining a fork 😄 |
|
@Mecharyry Thanks for your patience here. We're gearing up for a new release that includes web support, this PR will be included along with that work. We're aiming for that release within the next week or so. |
|
@Mecharyry FYI This has been included in v1.2.0-beta.1, we're expecting this to come out of beta next week 👍🏻 Thanks again for your contrubution! |
📋 Changes
Offering as an alternative to #206.
This aligns how Android and iOS parse dates. iOS is already formatting dates to
ISO8601Stringby directly taking theexpiresAtproperty, without modification. Android on the other hand is modifying thisexpiresAtby allowing it to be localised to whatever the systemsgetDefaultLocaleis.To solve the issues we have seen with
arthis ensures that all dates are using a consistent Locale ofLocale.US. The test addedcredentials_testis used just to illustrate the breaking changeis used to illustrate the issue with Arabic vs US English, but is actually fixed at the native level.📎 References
No references.
🎯 Testing
Put your device into
Arabicand attempt to use this library to log in. You'll notice that it works in this MR but doesn't onmain