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

[REQ][Elm] Remove DateTime in favour of Posix #3578

Closed
andys8 opened this issue Aug 7, 2019 · 7 comments
Closed

[REQ][Elm] Remove DateTime in favour of Posix #3578

andys8 opened this issue Aug 7, 2019 · 7 comments

Comments

@andys8
Copy link
Contributor

andys8 commented Aug 7, 2019

Is your feature request related to a problem? Please describe.

Generated code is obfuscating Posix with a new name DateTime.

Describe the solution you'd like

DateTime is an alias for Time.Posix. Since Time.Posix is now being used everywhere in Elm applications, it could make sense to remove the type alias for clarification. I think this is a left-over from the code base before Time.Posix was introduced.

@andys8 andys8 changed the title [REQ] Remove DateTime in favour of Posix [REQ][Elm] Remove DateTime in favour of Posix Aug 7, 2019
@wing328
Copy link
Member

wing328 commented Aug 7, 2019

@andys8 thanks for the suggestion.

cc @eriktim

@wing328
Copy link
Member

wing328 commented Aug 7, 2019

What about using --type-mapping with Time.Posix for the time being?

@andys8
Copy link
Contributor Author

andys8 commented Aug 7, 2019

@wing328 Could work, but isn't really necessary. Because it's a type alias it can be used the same way without the need to convert it.

The suggestion is mainly targetted towards avoiding confusion and using the type people know.

Thanks for the tipp.

@eriktim
Copy link
Contributor

eriktim commented Aug 9, 2019

I could rename the alias to Posix. That indeed makes sense. We can still use the decoder/encoder then. Any suggestions what to do with the DateOnly version then? Alias it as PosixDate? I do like the fact the type (alias) keeps them distinct.

@andys8
Copy link
Contributor Author

andys8 commented Aug 9, 2019

I was thinking of dropping the name. Is anything wrong with it or would you subject doing it like this?

module DateTime exposing (decoder, encode, toString)

import Iso8601
import Json.Decode as Decode exposing (Decoder)
import Json.Encode as Encode
import Result
import Time exposing (Posix)


decoder : Decoder Posix
decoder =
    Decode.string
        |> Decode.andThen decodeIsoString


encode : Posix -> Encode.Value
encode =
    Encode.string << toString


decodeIsoString : String -> Decoder Posix
decodeIsoString str =
    case Iso8601.toTime str of
        Result.Ok posix ->
            Decode.succeed posix

        Result.Err _ ->
            Decode.fail <| "Invalid date: " ++ str


toString : Posix -> String
toString =
    Iso8601.fromTime

@eriktim
Copy link
Contributor

eriktim commented Aug 9, 2019

There are two issues here:

  • To be honest I do not know how easy it would be to have the type (Posix) come from a different module than the de/encoders (DateTime) in the generator;
  • There is also DateOnly. I would like the record to have distinct types for dates with time and dates without time.

@eriktim
Copy link
Contributor

eriktim commented Jan 20, 2020

In the upcoming 5.0.0 release both date and date-time are now Posix values. Be aware that in case of date not the exact Posix value gets transmitted as the time is stripped off.

@eriktim eriktim closed this as completed Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants