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

Add a function for parsing ISO-8601 date time strings #15

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

akirak
Copy link

@akirak akirak commented Aug 16, 2020

This is currently a draft.

The test fails in my time zone, so I'll rework on this after #8 is merged. I want to follow styles of other test cases.

Perhaps I'll have to implement ts-parse-iso8601-fill as well.

@alphapapa alphapapa mentioned this pull request Aug 21, 2020
@andapony
Copy link

I think it would be better if ts-parse can handle ISO 8601 dates by itself, rather than having a separate API entry point. Is there an advantage in requiring the caller to distinguish between the formats itself?

@akirak
Copy link
Author

akirak commented Aug 21, 2020

I think it would be better if ts-parse can handle ISO 8601 dates by itself, rather than having a separate API entry point. Is there an advantage in requiring the caller to distinguish between the formats itself?

parse-time-string function in Emacs doesn't support ISO-8601, and ts-parse is a ts.el wrapper for the function.

It's generally hard to detect the time format for parsing, so I think it's better to be specify an explicit format.

@andapony
Copy link

It's generally hard to detect the time format for parsing, so I think it's better to be specify an explicit format.

That's sort of my point :-) It's hard, so it would be better for the library to be responsible for it rather than requiring clients to have to do the detection.

I guess I'm just puzzled why parse-time-string doesn't support ISO 8601. parse-iso8601-time-string falls back to parse-time-string if the string isn't an ISO 8601 format time. Given that, I don't really see why new code would ever call parse-time-string in preference to parse-iso8601-time-string.

@akirak
Copy link
Author

akirak commented Aug 22, 2020

Given that, I don't really see why new code would ever call parse-time-string in preference to parse-iso8601-time-string.

If a time string is obviously not ISO-8601, the user would want to skip extra computation of trying to parse it. Thus the user should know what date format is used. Otherwise, the smart parsing function should be named like ts-parse-smart.

@andapony
Copy link

Otherwise, the smart parsing function should be named like ts-parse-smart.

OK, I can see there's some value in that. It would then come down to deciding which case gets the default behavior (and short, simple name), and which case gets the more specific name. My preference would be to have ts-parse handle as many time formats as possible (i.e. including ISO 8601), and push the more optimized formats into ts-parse-non-iso8601 or something. Perhaps ts-parse could be extended with some kind of hints that help it avoid trying some formats?

@alphapapa
Copy link
Owner

Performance is a goal of this project, because some of its functions may be used in tight loops (e.g. in code that parses many timestamps in Org files), so the ts-parse function should be as simple as possible.

Even Emacs treats ISO-8601 as an explicitly known format that should be parsed intentionally, so I think it makes sense for us to do so as well.

If there's really a need for a flexible, "smart" parsing function that can handle both ISO-8601 and other formats supported by Emacs, then we could have such a function separately. But the user should make a conscious decision to use that function when necessary, so as not to penalize all other uses with the reduced performance.

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