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

Replace various type M env ms with common intersection #1744

Closed
sjakobi opened this issue Feb 4, 2016 · 2 comments
Closed

Replace various type M env ms with common intersection #1744

sjakobi opened this issue Feb 4, 2016 · 2 comments

Comments

@sjakobi
Copy link
Member

sjakobi commented Feb 4, 2016

The following files contain definitions of some type M env m which largely seem to be copies of each other:

src/Stack/Build.hs
src/Stack/Build/Execute.hs
src/Stack/Build/Installed.hs
src/Stack/Docker.hs
src/Stack/Nix.hs
src/Stack/SDist.hs

This leads to some redundancy and to less informative haddocks (example) because none of these types are exported.

I was wondering whether there would be a point in creating an alias of the intersection of these type constraints and exporting it (from Stack.Types.StackT maybe), so it shows in the haddocks. One idea for a name would be Common.

This idea conflicts with #1297.

@mgsloan
Copy link
Contributor

mgsloan commented Feb 5, 2016

Makes sense. I think the reasoning for having module local definitions of it is to make it easy to add a constraint to everything in the module. I also find it hard to follow, though.

I find the concrete approach to be the easiest to read. It provides better type errors, and better performance (nothing that really matters, though). It does sacrifice some flexibility, but I think in most cases we aren't using that flexibility. Then again, perhaps this is an aspect of stack's design that we don't want to sacrifice. Thoughts, @snoyberg @chrisdone ?

@mgsloan mgsloan added this to the Stabilization milestone Feb 5, 2016
@sjakobi
Copy link
Member Author

sjakobi commented Jul 14, 2016

I'm rather leaning towards the approach proposed in #1297 now.

@sjakobi sjakobi closed this as completed Jul 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants