pelotom / purescript-d3

PureScript bindings for D3
MIT License
86 stars 19 forks source link

Can we use type classes to eliminate need for `attr` and `attr'` functions? #11

Open tgdwyer opened 9 years ago

tgdwyer commented 9 years ago

Many thanks for creating purescript-d3, I'm having fun learning purescript by trying to convert js d3 demos.

I'm a purescript newbie just learning about type classes so sorry if I'm naive, but I wonder if it would be possible to create a typeclass for the possible arguments to the Selection attr functions, such that we can have a single overloaded attr that takes a string, a number or a function? I miss that aspect of the original javascript interface.

Seems simple enough to make a toy example - what am I missing?

class Helloable a where
  attr :: a -> String
instance helloNumber :: Helloable Number where
  attr n = "hello " ++ (show n)
instance helloString :: Helloable String where
  attr s = "hello " ++ s
instance helloFunc :: Helloable (String->String) where
  attr f = f " hello"

test = attr "there" ++ (attr 4) ++ (attr (\s -> "the function says " ++ s))

Thanks again!

tgdwyer commented 9 years ago

In fact, looking at the code for Selection.purs I see that there's the attr function is already overloaded for String and Number:

class AttrValue a
instance attrValNumber :: AttrValue Number
instance attrValString :: AttrValue String

Adding a third instance allows me to pass functions into the regular attr:

instance attrValFunc :: AttrValue (d->v)

But I guess it would be better to have some restrictions on the return value of the function...

pelotom commented 9 years ago

Hey, glad you like the library and thanks for your contributions!

This seems like a great idea, and I've played around with trying to do something like it, but I've run into problems that I don't know how to solve. As you mentioned, the tricky part is that you want to constrain the types involved... actually not just the return value v, but also the data type d, because it should match the currently bound data of the selection. So let's say we want to introduce a new method attr2:

class Existing s where
  -- ...
  attr2 :: forall d v. (AttrValue2 v d) => String -> v -> s d -> D3Eff (s d)
  -- ...

with the idea that it will be able to supersede attr and attr' (and ultimately attr'', but let's leave that aside for now). It has a constraint AttrValue2, which is parameterized not just by the value type v but also the data type d:

class AttrValue2 v d

So to have it do the duty of attr we just need to make any regular AttrValue also an AttrValue2 instance:

instance liftAttrVal :: (AttrValue v) => AttrValue2 v d

That seems to work fine; you can replace all instances of attr with attr2 in the examples and they still compile. The next step is to replace attr', so to do that we might add an instance:

instance attrValFun1 :: (AttrValue v) => AttrValue2 (d -> v) d

Unfortunately this doesn't seem to work as expected. purescript-d3 will compile by itself if you add this instance, but if you try to use it in the examples, replacing call sites of attr' with attr2, you'll run into trouble. Essentially it looks like the d parameter is not unifying with the data type, so you can pass functions to it from any type to a simple attribute value type. This seems like a compiler bug to me... I've noticed a bit of wonkiness in general with multiparameter typeclasses in PureScript. Anyway, that's the only obstacle I see with this approach. If this worked, then we could go on to define

instance attrValFun2 :: (AttrValue v) => AttrValue2 (d -> Number -> v) d

And thereby supersede attr'' as well. Then we could go and do the same for all the other foo'/foo'' methods in Graphics.D3.Selection! It would be a very nice simplification, for sure.

tgdwyer commented 9 years ago

Thanks for the response! Is it worth logging an issue about multi-parameter typeclasses over at purescript?

pelotom commented 9 years ago

Yeah, definitely worth filing a bug on it!

pelotom commented 9 years ago

If you make an issue, @-mention me so I can watch it and participate in the discussion.

tgdwyer commented 9 years ago

Actually, I'm going to pass on filing the bug on the purescript compiler myself. I had a quick go at making a minimal repro of the problem... but quickly realised when things started going wrong I couldn't tell whether it was my fault or the compiler. While it would be a great exercise for me to get to the bottom of it, I just don't have time at the moment. Sorry!