Skip to content

Move solver types to Distribution.Solver.* namespace #3406

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

Merged
merged 7 commits into from
May 6, 2016

Conversation

BardurArantsson
Copy link
Collaborator

This completes the move of all the solver types which the modular solver requires to Distribution.Solver.*.

Like last time there are few tiny modules, but again I've decided to go for consistency; see #3383.

This means that we now have:

$ grep -r Client cabal-install/Distribution/Solver|wc -l
0

which is to say that there are no longer any dependencies from the Solver namespace to the Client namespace. We should probably add a Travis check or something that ensures that none are added back accidentally.

@BardurArantsson
Copy link
Collaborator Author

-- Stability : provisional
-- Portability : portable
--
-- Common types for dependency resolution.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with removing the header, but it'd be nice to preserve the top comment about the purpose of the module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually removed it because the "purpose" isn't very clear any longer. The module is now a sort of weird mish-mash.

(I think this module should perhaps also be split, but I'm kind of waiting to see where migrating the Tests takes me... The module may become entirely redundant.)

@BardurArantsson
Copy link
Collaborator Author

Actually, perhaps we can drop the Travis check and just use an "internal library"?

@ezyang Do you think "internal library" support is robust enough at this point to move the Dependency.Solver.* bits into such a library? (I'm assuming tests will have access to such a library implicitly. Right?)

@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

LGTM, but would be nice to see some of the tiny modules recombined later on.

@BardurArantsson
Copy link
Collaborator Author

Yup, I fully intend to at some point, but I think it'll have to wait to see what happens when I try migrating the test cases, etc. I'm not sure we actually have the "full set" yet.

@BardurArantsson
Copy link
Collaborator Author

Hm, has Travis been disabled or something? I'm not seeing any "CI status" on this PR.

@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

Try tweaking something in the code (e.g. a comment), amend the last commit, and do a force-push.

@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

Actually don't do that, I found a way to fix this.

@BardurArantsson
Copy link
Collaborator Author

Check that. Thanks!

@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

OK, for some reason I can't re-trigger the Travis hook manually (only AppVeyor), but closing and reopening this PR should work.

@23Skidoo 23Skidoo closed this May 6, 2016
@23Skidoo 23Skidoo reopened this May 6, 2016
@23Skidoo 23Skidoo merged commit 030259c into haskell:master May 6, 2016
@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

Merged, thanks!

@ezyang
Copy link
Contributor

ezyang commented May 6, 2016

Re adding an internal library, I think we just need to merge #3356

@BardurArantsson
Copy link
Collaborator Author

OK, sounds good. I'll postpone the attempt until that's merged then :).

@23Skidoo
Copy link
Member

23Skidoo commented May 6, 2016

@ezyang

I think we just need to merge #3356

I think that all review comments in #3355 and #3356 have been addressed, so feel free to merge.

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.

3 participants