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

Fix parsing of ISO 8601 decimal fractions of a second #109

Merged
merged 7 commits into from
Sep 5, 2016

Conversation

erszcz
Copy link
Contributor

@erszcz erszcz commented Aug 19, 2016

It's based on top of #77 - please see the discussion there for some context. The solution here is not complete yet - any decimal fraction should be accepted, but 3 and 6 places after the comma are a good start, since milliseconds and microseconds are SI standardized submultiples of a second.

qrilka and others added 6 commits August 19, 2016 10:32
This is limited to milli- and microseconds interpreted
as 3 or 6 places after decimal comma.
All of the following, while valid according to the standard, won't be accepted:

- 2001-03-10T17:16:17.1Z
- 2001-03-10T17:16:17.12Z
- 2001-03-10T17:16:17.1234Z
- 2001-03-10T17:16:17.12345Z
- 2001-03-10T17:16:17.1234567Z
@tsloughter
Copy link
Member

Great. But this only accepts 3 or 6 decimal places? Not everything under 6?

@erszcz
Copy link
Contributor Author

erszcz commented Aug 22, 2016

Ok, I didn't realize it's not that much more effort to do full support. The caveats now:

  • 2001-03-10T17:16:17.1234Z will be parsed correctly, but when formatted won't look the same - formatting always prints the insignificant trailing zeros: 2001-03-10T17:16:17.123400Z
  • ec_date:tokenise/2 doesn't just tokenise - it also converts the fractional part into microseconds. I hope we can live with this slight name inaccuracy.

@erszcz
Copy link
Contributor Author

erszcz commented Sep 5, 2016

Any comments, required fixes, need for more tests? I'd be happy to see this merged, otherwise the library is missing quite a useful piece of functionality.

@tsloughter
Copy link
Member

Sorry for the delay, i think this is good now, will merge.

@tsloughter tsloughter merged commit 603441a into erlware:master Sep 5, 2016
@erszcz
Copy link
Contributor Author

erszcz commented Sep 6, 2016

Thanks!

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.

3 participants