rtfeldman / elm-iso8601-date-strings

Convert between ISO-8601 date strings and Time.Posix
https://package.elm-lang.org/packages/rtfeldman/elm-iso8601-date-strings/latest
BSD 3-Clause "New" or "Revised" License
34 stars 21 forks source link

Doesn't support ISO months #21

Open jarcane opened 5 years ago

jarcane commented 5 years ago

According to Wikipedia at least, it's valid to specify a month only, without a specific day, ie YYYY-MM only. I have an API I'm working with that returns months in this format.

Sadly this throws a parser error: [{ col = 8, problem = ExpectingSymbol "-", row = 1 }].

I can work around it of course by just tacking -01 onto the month, and maybe this is even the correct way you'd translate that format to a Posix date anyway, but I thought I'd point it out.

justinmimbs commented 5 years ago

Hi Richard,

I have a date package that parses all manner of ISO date strings (YYYY-MM-DD, YYYY-Www-D, YYYY-DDD, and their truncated forms, etc.), but not ISO date-time strings as this package does.

The date package doesn't currently expose its parser, but if it did then it could be used by this library for the date portion of the parsing (replacing this line and its dependencies). This package would then only need to parse the time and offset.

The parser is a Parser Date, and the Date type can be trivially converted to POSIX time (as done here). The parser has lots of tests, and it returns Parser.Problems on out-of-range parts like this package's parser does.

If you're interested please let me know, but I understand if you'd prefer not to add a dependency on another package.

Thanks!

Justin

jamesrweb commented 3 years ago

@justinmimbs are you still maintaining your date package?

justinmimbs commented 3 years ago

@justinmimbs are you still maintaining your date package?

Hi @jamesrweb, yes, I am.

jamesrweb commented 3 years ago

@justinmimbs since Richard didn't reply, I might give implementing this in here a go on the weekend, would simplify the code of this library a lot and fill the gaps in missing tests quite nicely. Any tips for the implementation as the author?

justinmimbs commented 3 years ago

@jamesrweb The Date package still doesn't expose its ISO date parser, but if it did (via, say, Date.isoParser : Parser Date), then it could be composed with the ISO time parsing in this package. The parsed Date, along with the optional time and offset, could then be converted to Posix with the help of Time.Extra.partsToPosix.

Does this help?

EDIT: I meant to say that if you'd like to try this approach, then I can expose the Parser Date from the Date package. I haven't done so before now as there didn't seem to be a need for it.

Thanks,

Justin

jamesrweb commented 3 years ago

@justinmimbs if I use your parser, why would I need the extra library and custom logic to be kept in this library? I mean custom logic is expected due to the API but for the parser specifically I mean.

I'm just thinking that if I'm to implement a better parser which IMO yours is, mostly due to test coverage among other things, I should also be able to drop it in and just one line it rather than requiring two packages and adding custom logic on this side.

Perhaps I've misunderstood something in your suggestion but I hope it makes sense to you what I'm getting at? Maybe you have some suggestion or clarification or just tell me if I'm just on another planet right now haha 😅

@justinmimbs Looking a bit deeper into things, I see that we could use fromIsoString in the toTime function and then if you exposed a dateToPosix : Date -> Time.Posix then I could just pass the returned Date into that for example. This way we don't need to have a parser exposed from your side but equally we can get rid of the iso8601 parser in this package too. So something like this could be possible for example:

toTime = String -> Time.Posix
toTime isoString =
    Date.dateToPosix (Date.fromIsoString isoString)

What do you think?

I guess the decode and encode functions should still be taken into account though since they are part of the current public API unless we want to create a breaking change or use more wrappers for the Date module. Just mulling this topic over internally for now and dumping my thoughts here for your consideration.