Add Compactable by FintanH · Pull Request #55 · FormationAI/dhall-bhat

FintanH · 2019-02-02T17:37:36Z

Compactable typeclass
fromSeparate -- default compact
fromCompact -- defaults separate
Added combinators

Notes:

I should probably add documentation (definitely)
I feel like there's some very repetitive patterns happening in the map, ap, bind, traverse functions. @sellout you probably have some tricks here 😛

joneshf

Wooo!!! Had a quick question about naming.

joneshf · 2019-02-02T17:47:46Z

@@ -0,0 +1,10 @@
+    let Applicative = ./../Applicative/Type


Should this (and the other similar expressions) be suffixed with Optional instead of Maybe: i.e. apOptional?

You are completely right. Was looking at the Haskell stuff 🙈

sellout

This looks great. I don’t have much opinion on the repetition stuff at the moment. But I think you need to merge in master to grab the circle config and get a green build.

FintanH · 2019-02-14T10:20:33Z

@sellout I don't think that's the CI issue.

# Blocked due to plan-no-credits-available Please purchase a new credit block then push a new commit or call the API to run a new build.

Looks like we don't have credits for this?

joneshf · 2019-02-05T16:42:47Z

-in    λ(f : Type → Type)
-    → { mapOptional :
-          ∀(a : Type) → ∀(b : Type) → (a → Optional b) → f a → f b
-      , mapEither :


What's the motivation behind removing mapEither? If it's about having one function to implement, we can derive mapOptional from mapEither, but we cannot derive mapEither from mapOptional. In that sense, it seems better to keep mapEither and have mapOptional be a derived function. Is there a different reason to remove mapEither?

I was wrong, you did derive mapEither! Forget I said anything.

We can derive mapEither from mapOptional it's here :) The motivation was that mapOptional is the canonical function when thinking about this class in terms of categories: Compactable f === KleisliFunctor Optional f where kmap :: (a -> m b) -> f a -> f b is specialised to (a -> Optional b) -> f a -> f b i.e. mapOptional 🎉!

This will make future integration with catewaul easier :)

Heh! I wrote my response before I saw the next comment 😄

sellout · 2019-02-14T15:43:51Z

Ah, well the previous failure was due to no CI config. This new one seems … wrong. This is an OSS project, I thought those wouldn’t require “credits”?

FintanH · 2019-02-14T15:46:04Z

Yeh I've no idea, should we ask ops?

sellout · 2019-02-16T02:40:36Z

@@ -0,0 +1,49 @@
+{-
+
+mapEither maps a function to Either over the `f` and `separate`s `Left`s from `Right`s.


This says mapEither, but the file is separate. I would change this comment to say something like “This expression maps …” to make it less likely to get out of sync.

sellout · 2019-02-16T02:41:19Z

+```
+    let E = constructors (./Either/Type Natural Natural)
+
+in  ./Compactable/mapEither


Same here. Obviously harder to avoid the name. I guess we need like dhall-doctest now, right 😁

sellout · 2019-02-16T02:42:33Z

@@ -0,0 +1,31 @@
+{-
+mapOptional : (a -> Optional b) -> Either m a -> Either m b


I don’t think this is useful documentation, especially since this is a type class instance.

Thanks for catching, I actually just put it there to help me implement it 😅

sellout · 2019-02-16T02:44:35Z

+
+in  let E = constructors (Either Text Natural)
+
+in  ./Either/flipEither Text Natural (E.Left "ohno")


Should probably just rename this to flip. Also, it’s probably type-classable, like Bifunctor-ish?

Did a hoogle: https://hackage.haskell.org/package/invertible-0.2.0.5/docs/Control-Invertible-BiArrow.html#v:invert

I've leave it for now though and just rename to flip

Actually, Either isn't an instance. Need to keep looking :)

Braided it would seem https://hackage.haskell.org/package/hs-functors-0.1.3.0/docs/src/Data.Bifunctor.Braided.html

sellout · 2019-02-16T02:46:19Z

+{-
+
+Compactable serves as an abstraction over filtering and partitioning, using two functions
+compact and separate, respectively.


This makes it sound like the type class still has multiple methods. I would rewrite this to discuss mapOptional instead, and then move the other comments to the respective expressions.

sellout · 2019-02-16T02:46:42Z

@@ -0,0 +1,30 @@
+{-
+apEither maps an `f` of functions to Eithers over the `f a` and `separate`s `Left`s from `Right`s.


Get rid of apEither in the comment here.

sellout · 2019-02-16T02:47:00Z

@@ -0,0 +1,31 @@
+{-
+
+apOptional maps a `f` of functions to Optionals over the `f` and `compact`s away the `None` values.


Get rid of “apOptional” here.

sellout · 2019-02-16T02:47:37Z

@@ -0,0 +1,44 @@
+{-
+
+bindEither binds to a `f` of Optional over the `f a` and `separate`s `Left`s and `Right`s.


Same, etc. for other files.

sellout · 2019-02-16T02:47:56Z

@@ -0,0 +1,23 @@
+{-
+
+mapOptional maps a function to Optionals over the `f` and `compact`s away the `None` values.


This claims to be mapOptional, but isn’t.

sellout · 2019-02-16T02:49:23Z

Ah crap … apparently I left a ton of comments on an old commit sigh.

sellout · 2019-02-16T02:50:36Z

I guess like two of my comments are still valid 🤷🏼‍♂️

FintanH added 13 commits February 2, 2019 10:48

Clean up Monad Type

24d71e1

Add flipEither and hush

c21b644

Use [] annotation for None

8460fbb

Add base type class for Compactable and from functions

333e454

Use None instead of []

74b40fa

Run linter

c0eaf24

Add Compactable combinators

bb8dbd5

Use anonymous tuple syntax

53e0a1f

Clean up compactable representation

86b5f2a

List compactable instance

eb27c3c

Fix fromCompact

5473be0

Add Optional instance

9e30147

Added Either instance

8dca13e

FintanH added the Open Source Labelling open source contributions label Feb 2, 2019

FintanH assigned sellout Feb 2, 2019

FintanH requested a review from sellout February 2, 2019 17:37

joneshf reviewed Feb 2, 2019

View reviewed changes

FintanH added 11 commits February 2, 2019 20:04

Rename Maybe to Optional

b744ae7

Fix fromCompact and annotate instances

ad3b3fa

Document Compactable/Type

d91d645

Document from functions

4e105fd

Fix spacing, add docs and examples to map and ap functions

c89d4c4

Fix ap docs

582df5e

Add bind docs

f83ae86

Document flipEither and hush

780de73

Make typeclass method be mapOptional and mapEither

12ee10e

Only use mapOptional. Clean up definitions

598e210

Update docs and found some type errors

74036eb

sellout approved these changes Feb 13, 2019

View reviewed changes

Merge branch 'master' into fintan/compactable

9205845

joneshf reviewed Feb 14, 2019

View reviewed changes

sellout suggested changes Feb 16, 2019

View reviewed changes

Move flipEither to flip. Remove not needed doc

7429abc

		@@ -0,0 +1,49 @@
		{-

		mapEither maps a function to Either over the `f` and `separate`s `Left`s from `Right`s.

		@@ -0,0 +1,31 @@
		{-
		mapOptional : (a -> Optional b) -> Either m a -> Either m b


		in let E = constructors (Either Text Natural)

		in ./Either/flipEither Text Natural (E.Left "ohno")

		@@ -0,0 +1,30 @@
		{-
		apEither maps an `f` of functions to Eithers over the `f a` and `separate`s `Left`s from `Right`s.

		@@ -0,0 +1,31 @@
		{-

		apOptional maps a `f` of functions to Optionals over the `f` and `compact`s away the `None` values.

		@@ -0,0 +1,44 @@
		{-

		bindEither binds to a `f` of Optional over the `f a` and `separate`s `Left`s and `Right`s.

		@@ -0,0 +1,23 @@
		{-

		mapOptional maps a function to Optionals over the `f` and `compact`s away the `None` values.

Conversation

FintanH commented Feb 2, 2019

Uh oh!

joneshf 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!

sellout left a comment

Choose a reason for hiding this comment

Uh oh!

FintanH commented Feb 14, 2019

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!

sellout commented Feb 14, 2019

Uh oh!

FintanH commented Feb 14, 2019

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!

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!

sellout commented Feb 16, 2019

Uh oh!

sellout commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants