Add Interval #52

safareli · 2017-04-03T13:32:42Z

No description provided.

safareli · 2017-04-03T13:51:27Z

src/Data/Interval/Interval.purs

+
+data RecurringInterval a = RecurringInterval (Maybe Int) (Interval a)
+data Duration
+  = DurationWeek Week


Duration could be in two formats [PnnW] or [PnnYnnMnnDTnnHnnMnnS].

As I understand one could rewrite for example [P3W] into [P21D] and vise versa (1 week is exactly 7 days isn't it?)

So we could ignore week format as it could be expressed with full date format, but in that case it would be impossible to take some string representing ISO Duration, parse it then serialize it and get same string.

but isoInterval == serialize (parse isoInterval) will not be true for all ISO Duration as currently we don't know if a component of Duration the interval was specified (a component could be omitted if it equals 0) if we want for it to be true we should store Maybe Int as values of components instead of Int, but if we don't do that I think we could also remove this variant and during parsing if we see week format parse it as normal duration.

safareli · 2017-04-03T13:56:36Z

This haskell lib is related but Duration representation there is somewhat complex https://github.com/meteogrid/iso8601-duration/tree/master/src/Data/Time/ISO8601

safareli · 2017-04-03T14:13:03Z

src/Data/Interval/Interval.purs

+
+import Prelude
+
+data Interval a


a is allowed to be Date, DateTime, LocalValue Date and LocalValue DateTime (only Time is not allowed)

4.4.4 Complete representations
4.4.4.1 Representations of time intervals identified by start and end

When the application identifies the need for a complete representation of a time interval, identified by its start and its end, it shall use an expression in accordance with 4.4.2 combining any two complete date and time of day representations as defined in 4.3.2, provided that the resulting expression is either consistently in basic format or consistently in extended format

4.3.2 Complete representations
The time elements of a date and time of day expression shall be written in the following sequence:
a) For calendar dates: year – month – day of the month – time designator – hour – minute – second – zone designator
b) For ordinal dates: year – day of the year – time designator – hour – minute – second – zone designator
c) For week dates: year – week designator – week – day of the week – time designator – hour – minute – second – zone designator
....

safareli · 2017-04-03T14:14:01Z

Link to:

garyb · 2017-04-03T17:09:11Z

src/Data/Interval/Interval.purs

+  ( Duration
+  , Interval
+  , RecurringInterval
+  ) where


With these exports, it's not possible to actually construct any of the interval values, to export the constructors to you need to list (..) after the type; so Duration(..), and so on.

Having said that, we might not want to export the constructors, since if I read what you're saying rightly below, we want to make it impossible to construct an Interval Time - that means we'll have to provide "smart constructors" (functions for each of the valid constructions), and then only export the types, as you have here.

safareli · 2017-04-04T16:05:21Z

src/Data/Interval/Interval.purs

+data Duration = Duration DurationIn
+type DurationIn = List (Tuple DurationComponent Number)
+
+derive instance eqDuration ∷ Eq Duration


TODO make hours 24 == day 1

safareli · 2017-04-05T17:14:27Z

src/Data/Interval/Interval.purs

+  }
+
+mkDuration ∷ DurationView → Duration
+mkDuration d = Duration $


TODO remove mkDuration and DurationView as it's just:
mkDuration d = year d.year <> month d.month <> day d.day <> hours d.hours <> minutes d.minutes <> seconds d.seconds

safareli · 2017-04-25T12:24:00Z

src/Data/Interval/Interval.purs

+
+
+data Duration = Duration (Map.Map DurationComponent Number)
+-- TODO `day 1 == hours 24`


I think derived Eq instance is fine and we don't need to say that day 1 == hours 24 as they are build using different components and technically could be different (leap second).

If we really want we can, add other function which assumes day 1 == hours 24 and etc.

safareli · 2017-04-25T12:26:54Z

@garyb what would you suggest to validate that instances for RecuringInterval and Interval are correct?

cryogenian · 2017-04-26T20:25:42Z

package.json

-    "pulp": "^10.0.4",
-    "purescript-psa": "^0.5.0-rc.1",
+    "pulp": "^11.0.x",
+    "purescript": "0.11.x",


It's same. do you prefer 0 ?

oh I forgot ^

cryogenian · 2017-04-26T20:37:45Z

src/Data/Interval/Interval.purs

+instance extendRecurringInterval ∷ Extend (RecurringInterval d) where
+  extend f a@(RecurringInterval n i) = RecurringInterval n (extend (const $ f a) i )
+
+data Interval d a


Can't this be a bit more typesafe? Like Interval String Unit is valid type, but it has no sense

format and unformat are "safe" i.e. they only take Interval IsoDuration DateTime but someone someone might have Interval String Unit and use bimap to transform it into something format-able.

Or one might have Interval (Maybe IsoDuration) (Maybe DateTime) and then use bisequenceto transform it into Maybe (Interval IsoDuration DateTime)

I'm just a bit curious about this datatype because it has tons of typeclass instances. It looks like it could be something more generic. E.g. These d (These a a) or something like this. Same for RecurringInterval it looks like much more generic type than just RecurringInterval, it's almost the Const for bifunctors, right?

@garyb What do you think?

I see what you're saying, but I think having a specialised type here is probably helpful in this case - it adds some context to what the meanings of the fields are (assuming a and d aren't populated with nonsensical types). When you pattern match on a StartDuration s d it tells you a bit more about the intention of the code than a Both d (This s) kinda thing.

@paf31 do you have any thoughts about alternative/better representations for this kind of thing?

DateTime is not functor/bifunctor/extend around Date|Time 😛

I don't mind having specialized type for this though, just want to be sure that there is as few ways of writing incorrect code as possible.

-- BiTag to adds tag/metadata to bifunctor newtype BiTag t p a b = BiTag (Tuple t (p a b)) -- Just made this up. BiTag might be more useful than Those data Those d a = Those a a | Both a (Either d d) | That d newtype Interval d a = Interval (Those d a) newtype RecuringInterval d a = RecuringInterval (BiTag (Maybe Int) Interval d a)

@safareli 💯

but I suppose data BiTag t p a b = BiTag t (p a b) or newtype BiTag t p a b = BiTag (Tuple t (p a b)):)

safareli · 2017-04-27T14:13:11Z

src/Data/Interval/Interval.purs

+module Data.Interval
+  ( Interval(..)
+  , RecurringInterval(..)
+  , IsoDuration


IsoDuration and related stuff could actually be moved to purescript-formatters.

let's leave this as is for now.

safareli · 2017-06-23T10:44:01Z

some helper function could be adopted from here
https://github.com/arnau/ISO8601/blob/master/spec/iso8601/duration_spec.rb

safareli · 2017-06-23T10:48:07Z

@garyb I think this could be merged, If you are fine with changes.

garyb

A whole bunch of minor picky comments, but I think it's almost there!

garyb · 2017-06-23T11:07:57Z

src/Data/Interval/Interval.purs

+  show (IsoDuration d)= "(IsoDuration " <> show d <> ")"
+
+
+newtype Duration = Duration (Map.Map DurationComponent Number)


Can we move this & associated functions into Data.Interval.Duration and re-export instead please?

garyb · 2017-06-23T11:08:11Z

src/Data/Interval/Interval.purs

+newtype Duration = Duration (Map.Map DurationComponent Number)
+
+derive instance eqDuration ∷ Eq Duration
+derive instance newtypeDuration ∷ Newtype Duration _


No Ord instance?

garyb · 2017-06-23T11:09:00Z

src/Data/Interval/Interval.purs

+derive instance newtypeDuration ∷ Newtype Duration _
+
+instance showDuration ∷ Show Duration where
+  show (Duration d)= "(Duration " <> show d <> ")"


Nitpick: space before =

garyb · 2017-06-23T11:09:53Z

src/Data/Interval/Interval.purs

+
+durationFromComponent ∷ DurationComponent → Number → Duration
+-- durationFromComponent _ 0.0 = mempty
+durationFromComponent k v= Duration $ Map.singleton k v


Does this commented line need restoring?

Also another space-before-= nitpick 😄

garyb · 2017-06-23T11:13:35Z

src/Data/Interval/Interval.purs

+newtype IsoDuration = IsoDuration Duration
+derive instance eqIsoDuration ∷ Eq IsoDuration
+instance showIsoDuration ∷ Show IsoDuration where
+  show (IsoDuration d)= "(IsoDuration " <> show d <> ")"


Can we move this & associated functions into Data.Interval.Duration.Iso please? I wouldn't necessarily re-export either, since it's a more specific thing than "intervals in general".

garyb · 2017-06-23T11:23:46Z

src/Data/Interval/Interval.purs

+isFractional a = Math.floor a /= a
+
+isValidIsoDuration ∷ Duration → Boolean
+isValidIsoDuration (Duration m) = (not $ Map.isEmpty m) && (hasValidFractionalUse m)


Parens are unnecessary here, and not applies to functions that return booleans as well as booleans, so not Map.isEmpty m && hasValidFractionalUse m should work.

"not applies to functions that return booleans" fantastic ✨
✅

garyb · 2017-06-23T11:24:21Z

src/Data/Interval/Interval.purs

+      >>> (\vals -> fold (vals =>> validateFractionalUse) <> positiveNums vals)
+      >>> extract
+    validateFractionalUse vals = Conj $ case vals of
+      (Tuple _ n):as | isFractional n → foldMap (snd >>> Additive) as == mempty


There's a few usages of unicode, like the arrow here, but we don't use unicode in core/contrib.

garyb · 2017-06-23T11:27:01Z

src/Data/Interval/Interval.purs

+
+
+week ∷ Number → Duration
+week = durationFromComponent Day <<< (_ * 7.0)


Maybe we should just have a Week component too. It can be represented by 7 days also, but that applies to any time measure up to Month being representable just by Milliseconds, so it wouldn't do any harm to add Week as that will allow us to represent P1W accurately too.

garyb · 2017-06-23T11:27:18Z

src/Data/Interval/Interval.purs

+instance monoidDuration ∷ Monoid Duration where
+  mempty = Duration mempty
+
+data DurationComponent = Year | Month | Day | Hour | Minute | Second


I'd reverse the order of these constructors so the derived Ord instance is more sensible.

garyb · 2017-06-23T11:30:32Z

src/Data/Interval/Interval.purs

+  bisequence = bisequenceDefault
+
+instance extendRecurringInterval ∷ Extend (RecurringInterval d) where
+  extend f a@(RecurringInterval n i) = RecurringInterval n (extend (const $ f a) i )


Extra space before closing paren.

This instance looks a little odd to me too! I don't think it breaks any laws though.

space ✅

extend :: (RecurringInterval d a -> b) -> RecurringInterval d a -> RecurringInterval d b extend f a@(RecurringInterval n i) = RecurringInterval n (extend (const $ f a) i )

n is fixed and can't change. one thing which changes is i :: Interval d a
at first we call the f to obtain b. then we can delegate to
extend of i :: Interval d a to get Interval d b. how? we already have
value of desired type f a :: b so we can just call extend on i with
const (f a) :: Interval d a -> b and we get Interval d b.

Extend instance for Interval and RecurringInterval might not be that useful tho. but someone might use it.

we can remove it and create an issue add Extend instance for Interval and RecurringInterval, in which and ask for a use case for it to be added :d

- move Interval.purs up one level - move Duration and IsoDuration parts away

This way InvalidWeekComponentUsage will be first if it is present in errors

garyb

Looks great, thanks!

safareli added 2 commits April 3, 2017 17:31

WIP: Add Interval

593e364

remove unused imports

37b6f36

safareli commented Apr 3, 2017

View reviewed changes

remove dublicats

fac9d4b

garyb reviewed Apr 3, 2017

View reviewed changes

safareli added 2 commits April 4, 2017 17:28

update Duration

0805ce4

add couple instances for Duration and Interval

5c49ea6

safareli commented Apr 4, 2017

View reviewed changes

reorder code

d7dec5a

safareli commented Apr 5, 2017

View reviewed changes

safareli added 2 commits April 5, 2017 21:34

remove mkDuration DurationView and Milliseconds component

62bf305

fis ord instance of DurationComponent

089d9ef

safareli mentioned this pull request Apr 5, 2017

Interval formatters purescript-contrib/purescript-formatters#17

Merged

safareli added 3 commits April 6, 2017 16:50

make Interval Bifunctor

266d481

add Bifoldable for Interval

0a7b628

add IsoDuration; use Map instead of List Tuple

0ceb8eb

safareli force-pushed the interval branch from 51b7480 to 0ceb8eb Compare April 6, 2017 16:29

safareli added 4 commits April 19, 2017 01:17

derive instances for RecurringInterval

c6cbe32

Merge branch 'master' into interval

42bbfb7

fix derivations

503c6f8

add Bitraversable and Eq instances

5d06e53

safareli commented Apr 25, 2017

View reviewed changes

safareli added 2 commits April 25, 2017 19:24

export Duration and DurationComponent

d2acbd6

make sure IsoDuraiton is not empty

5f66a89

cryogenian reviewed Apr 26, 2017

View reviewed changes

add ^ to PS version

ea6396c

revert travis changes

e95b67f

safareli commented Apr 27, 2017

View reviewed changes

safareli added 6 commits May 16, 2017 09:01

make Duration new type and derive Newype

34640c6

make duration components singular

809a234

reverse duration component order

3eda0dd

make isoDuration newtype

720cab7

allow only positive values in duration

5e7b7be

remove some TODOs

fbd30c4

safareli changed the title ~~WIP: Add Interval~~ Add Interval Jun 23, 2017

garyb requested changes Jun 23, 2017

View reviewed changes

safareli added 9 commits June 23, 2017 16:29

fix spaces and unicodes

359e28a

add ord instances

355eb63

fix spacing, parens and $ usage

dfca687

rename JustDuration to DurationOnly

c3817cc

split Interval

d9457d2

- move Interval.purs up one level - move Duration and IsoDuration parts away

reverse DurationComponent order

447406a

add Week component to Duration; refactor Duration.Iso

d829b8c

move checkWeekUsage up add tests

08c4d82

This way InvalidWeekComponentUsage will be first if it is present in errors

use NonEmptyList instead of NonEmpty Array; add prettyError

84d8b2c

garyb approved these changes Jun 26, 2017

View reviewed changes

garyb merged commit 3cb01cd into purescript:master Jun 26, 2017



		data Duration = Duration (Map.Map DurationComponent Number)
		-- TODO `day 1 == hours 24`

		show (IsoDuration d)= "(IsoDuration " <> show d <> ")"


		newtype Duration = Duration (Map.Map DurationComponent Number)



		week ∷ Number → Duration
		week = durationFromComponent Day <<< (_ * 7.0)

Add Interval #52

Add Interval #52

Uh oh!

Conversation

safareli commented Apr 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli commented Apr 3, 2017

Uh oh!

safareli Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli commented Apr 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli commented Jun 23, 2017

Uh oh!

safareli commented Jun 23, 2017

Uh oh!

garyb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safareli Apr 3, 2017 •

edited

Loading

safareli commented Apr 3, 2017 •

edited

Loading

safareli Apr 25, 2017 •

edited

Loading

safareli Apr 27, 2017 •

edited

Loading

safareli Apr 27, 2017 •

edited

Loading