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

add b->a instance for Alignable #133

Merged
merged 2 commits into from
Oct 24, 2013

Conversation

cscheid
Copy link
Contributor

@cscheid cscheid commented Oct 23, 2013

I'm keeping the failed attempt to get an instance via defaultBoundary as comments, mostly so you can see what I was trying to get at. I don't think something like that will work easily, but with this pull request, diagrams-lib emits warnings during compilation, which is bad.

Ideas?

@byorgey
Copy link
Member

byorgey commented Oct 23, 2013

Thanks, I'll look at this more carefully soon. I have to page back in the way Alignable works first.

@byorgey
Copy link
Member

byorgey commented Oct 24, 2013

Aha, I think I understand a bit better now. All the other "derived" Alignable instances (for [a], Set a, Map a, etc.) are for finite containers, and work by aligning to the combined boundary of all the contained Alignable things. This can't work for functions, however, since a function may be an "infinite" container (if its domain is infinite). Instead of aligning to the combined boundary, we just want to align each output individually. So there is no sensible defaultBoundary for functions. We can probably just implement it as \_ _ -> origin, with an explanatory note; since we don't use it for implementing alignBy it doesn't really matter (unless you try to do something like, say, align Sets of functions... which would simply have no effect). The given implementation of alignBy looks perfect.

@cscheid
Copy link
Contributor Author

cscheid commented Oct 24, 2013

Ok, see if that's good enough - it at least compiles cleanly for me.

@byorgey
Copy link
Member

byorgey commented Oct 24, 2013

Looks good -- thanks!

byorgey added a commit that referenced this pull request Oct 24, 2013
@byorgey byorgey merged commit f0788fc into diagrams:master Oct 24, 2013
@cscheid cscheid deleted the Alignable-b-to-a-instance branch October 24, 2013 18:10
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

Successfully merging this pull request may close these issues.

None yet

2 participants