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

Attributes aren't type safe #84

Open
cchalmers opened this issue Apr 3, 2015 · 7 comments
Open

Attributes aren't type safe #84

cchalmers opened this issue Apr 3, 2015 · 7 comments

Comments

@cchalmers
Copy link
Member

There's three different types of Attribute but all attributes can be used with the standard Attribute.

It's an easy mistake to wrap up an attribute incorrectly, I've seen (and done) it a few times. After spending over an hour debugging to find out splitAttr does this too I want this to be fixed :)

One way is to use data kinds:

data AttrKind = Attr | MAttr | TAttr

class AttributesClass where
  type AttrType :: AttrKind

applyAttribute :: (AttributeClass a, AttrType a ~ Attr, HasStyle d) => a -> d -> d
applyTAttribute :: (AttributeClass a, AttrType a ~ TAttr, HasStyle d) => a -> d -> d
atAttr :: (AttributeClass a, AttrType a ~ Attr) :: Lens' (Style v n) (Maybe a)
@jeffreyrosenbluth
Copy link
Member

It might be interesting to take a pause and think about how we handle styles and attributes. In diagrams we basically have 2 extensible features, primitives and attributes. Primitives are statically typed but attributes are dynamic, see https://wiki.haskell.org/Diagrams/Dev/Expression for a discussion about making primitives dynamic.

It seems we've settled on statically typed primitives. I wonder whether we should think about treating attributes like primitives and consider the pros and cons. I doubt we will actually want to do this, but I think it is a good time to consider it.

@cchalmers
Copy link
Member Author

If we want to implement fading (using the luminosity of a rendered diagram as an alpha channel) or pattern fills (tiling a diagram to use as a path's fill) we'd need to add b annotations to Attribute and Style.

I'm leaning towards dynamic primitives. Right now the only case where it's useful is for images but I could see 3D backends being more selective with features they support. I'd still be happy with issuing warnings when a backend gets something it doesn't support.

That said I think the original issue is separate and can be implemented without (hopefully) breaking user code or backends, it just helps us make less mistakes in the library code.

@jeffreyrosenbluth
Copy link
Member

@cchalmers take a look at the now bit-rotted dynamic branches of -core, -lib and -svg were I played around with dynamic primitives.

@cchalmers
Copy link
Member Author

Yeah, the more I think about it the more I'm in favour of ditching the b. Changing the code doesn't seem too hard, the bigger problem would be documenting / reporting errors / changing Diagram alias again.

@jeffreyrosenbluth
Copy link
Member

I wonder if it makes sense to try and merge master into dynamic. Or if it's easier just to start from scratch. @cchalmers before you do any more work on it, we should see if there is a consensus amongst the team. If I remember correctly, when I wrote this others were leaning towards keeping the b

@byorgey
Copy link
Member

byorgey commented Apr 23, 2015

Static vs dynamic primitives aside, I'm definitely in favor of making attributes more type safe. I've definitely stumbled over (read: had to debug) this too. The proposed solution looks good to me. DataKinds was introduced in GHC 7.4 so we are certainly good to start depending on it.

@byorgey
Copy link
Member

byorgey commented Apr 23, 2015

(For the record, I am still torn on static vs dynamic primitives.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants