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 currentDate as a task in Date.elm #276

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ThomasWeiser
Contributor

ThomasWeiser commented Jun 13, 2015

We have Date and we have Task, so IMHO we should have a task to get the current date.

This PR implements

Date.currentDate : Task () Date

Of course it could be named differently or could be put into another module. The chosen ones are just my preference.

I didn't write a test within the core library as there are no tests for tasks up to now.
But I did test the code for my own and it works like expected.

Show outdated Hide outdated src/Native/Date.js
@@ -27,6 +28,12 @@ Elm.Native.Date.make = function(localRuntime) {
["Jan", "Feb", "Mar", "Apr", "May", "Jun",
"Jul", "Aug", "Sep", "Oct", "Nov", "Dec"];
currentDate = Task .asyncFunction (

This comment has been minimized.

@rtfeldman

rtfeldman Jul 21, 2015

Member

seems unlikely that this compiles... 😉

@rtfeldman

rtfeldman Jul 21, 2015

Member

seems unlikely that this compiles... 😉

This comment has been minimized.

@ThomasWeiser

ThomasWeiser Jul 21, 2015

Contributor

Thanks, this should read var currentDate = ..., so I missed the var.
Or do you have other concerns?

@ThomasWeiser

ThomasWeiser Jul 21, 2015

Contributor

Thanks, this should read var currentDate = ..., so I missed the var.
Or do you have other concerns?

@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jul 21, 2015

Contributor

Here is a simple test app:

import Html
import Date exposing (Date)
import Time
import Task

port execute : Task.Task () ()
port execute =
  Date.current `Task.andThen`
  \date -> Signal.send result.address date

result : Signal.Mailbox Date
result = Signal.mailbox (Date.fromTime 0)

main = Signal.map
  (\date -> Html.text <| toString (Date.toTime date))
  result.signal
Contributor

ThomasWeiser commented Jul 21, 2015

Here is a simple test app:

import Html
import Date exposing (Date)
import Time
import Task

port execute : Task.Task () ()
port execute =
  Date.current `Task.andThen`
  \date -> Signal.send result.address date

result : Signal.Mailbox Date
result = Signal.mailbox (Date.fromTime 0)

main = Signal.map
  (\date -> Html.text <| toString (Date.toTime date))
  result.signal
@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jul 21, 2015

Contributor

@dnalot

Why a task?

How would you provide the ability to get the current date? Maybe I am missing something.

Granted, there is already Time.timestamp, which does the same in principal, but

  • it's not documented that timestamps actually represent real time/date.
  • it's cumbersome to define an extra signal when you need the current date in a task (e.g. send the local time to a server).

IMHO, the core libarary should provide both,

  • timestamp as a signal function for needs within the pure Elm-Architecture
  • currentDate as a task for needs in the imperative task-based world
Contributor

ThomasWeiser commented Jul 21, 2015

@dnalot

Why a task?

How would you provide the ability to get the current date? Maybe I am missing something.

Granted, there is already Time.timestamp, which does the same in principal, but

  • it's not documented that timestamps actually represent real time/date.
  • it's cumbersome to define an extra signal when you need the current date in a task (e.g. send the local time to a server).

IMHO, the core libarary should provide both,

  • timestamp as a signal function for needs within the pure Elm-Architecture
  • currentDate as a task for needs in the imperative task-based world
@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 21, 2015

Ha I forgot time isn't referentially transparent. Your implementation looks straightforward! It seems essential for debugging and countdown timers. What's your use case? Feedback: functions generally shouldn't restate their package name. I like JS's Date.now but maybe there are alternatives.

texastoland commented Jul 21, 2015

Ha I forgot time isn't referentially transparent. Your implementation looks straightforward! It seems essential for debugging and countdown timers. What's your use case? Feedback: functions generally shouldn't restate their package name. I like JS's Date.now but maybe there are alternatives.

@ThomasWeiser

This comment has been minimized.

Show comment
Hide comment
@ThomasWeiser

ThomasWeiser Jul 22, 2015

Contributor

Thanks @dnalot, I renamed the function to Date.current (but Date.now would be equally handy).

Contributor

ThomasWeiser commented Jul 22, 2015

Thanks @dnalot, I renamed the function to Date.current (but Date.now would be equally handy).

@texastoland

This comment has been minimized.

Show comment
Hide comment
@texastoland

texastoland Jul 22, 2015

current is only slightly odd being an adjective. @evancz recommended in #303 vetting new functionality in separate libraries. I didn't see a good fit in existing packages or unpublished repos but it sounds like a superb idea for a new elm-date-extra! @rehno-lindeque might have something to add.

texastoland commented Jul 22, 2015

current is only slightly odd being an adjective. @evancz recommended in #303 vetting new functionality in separate libraries. I didn't see a good fit in existing packages or unpublished repos but it sounds like a superb idea for a new elm-date-extra! @rehno-lindeque might have something to add.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 22, 2015

Member

To get this done in a timely manor, I think it makes sense to make a package for time that has nice things in it. We will do a systematic review of these libraries at some point and bring them into core. (plan outlined here)

Member

evancz commented Jul 22, 2015

To get this done in a timely manor, I think it makes sense to make a package for time that has nice things in it. We will do a systematic review of these libraries at some point and bring them into core. (plan outlined here)

@evancz evancz closed this Jul 22, 2015

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 22, 2015

Member

Sorry for the long delay on getting you a response @ThomasWeiser. It is hard to juggle all the stuff going on these days!

Member

evancz commented Jul 22, 2015

Sorry for the long delay on getting you a response @ThomasWeiser. It is hard to juggle all the stuff going on these days!

@rehno-lindeque

This comment has been minimized.

Show comment
Hide comment
@rehno-lindeque

rehno-lindeque Jul 22, 2015

Contributor

An -extra package sounds like a fine idea. @ThomasWeiser we have a bunch of them here https://github.com/circuithub?query=elm- if you're looking for any examples

Contributor

rehno-lindeque commented Jul 22, 2015

An -extra package sounds like a fine idea. @ThomasWeiser we have a bunch of them here https://github.com/circuithub?query=elm- if you're looking for any examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment