search

diagrams / diagrams-contrib

User-contributed extensions to diagrams
BSD 3-Clause "New" or "Revised" License
27 stars 31 forks source link

Tree.hs: hide Empty from Diagrams.Prelude #57

Closed akhra closed 8 years ago

akhra commented 8 years ago

Fixes #56.

byorgey commented 8 years ago

This is the simplest fix, but anyone using Diagrams.TwoD.Layout.Tree and importing Diagrams.Prelude will still run into the same issue. Another alternative would be to hide Empty from the lens import so it is not re-exported from Diagrams.Prelude. We do this for some other things from lens, see https://github.com/diagrams/diagrams-lib/blob/master/src/Diagrams/Prelude.hs#L81. Anyone have any thoughts on this?

akhra commented 8 years ago

I went with this fix because it can't break existing code. Agreed that the other option is superior in general, but it warrants a bump to 1.4 on the wild chance that someone is actually using that Prelude export. Not something I was comfortable tossing into a cold-call pull request. :)

akhra commented 8 years ago

Just to be comprehensive about it: the third option is to rename the local one e.g. BTEmpty. That breaks code too, but does have the end result of no toys taken away for a given set of imports.

cchalmers commented 8 years ago

I vote we rename it to BEmpty, I can see the Empty pattern in lens being useful. We could also add an AsEmpty instance so if they enable PatternSynonyms they can still use Empty for the empty tree.

byorgey commented 8 years ago

Hmm, the third option sounds the best, but that also technically requires a major version bump. =( I propose we just merge this as-is, to get a buildable version out quickly, and then implement the third option to be released at some point in the future when we put out diagrams 1.4.

cchalmers commented 8 years ago

Sounds good to me.

byorgey commented 8 years ago

Released as diagrams-contrib-1.3.0.7.