Refactor to remove code duplication and be more pipeliney #1991
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This way, callers who want source.root_package() don't have to remember to call source.update() before that. Since source.update() is a noop if the source has already been updated, there's not a reason I could see to raise an error instead of just calling it. The one remaining place that calls source.root_package() that is still calling source.update() immediately before is in bin/read_manifest, where the errors from update() are mapped to CliErrors.
There are many places where both source and its root_package() are used, but in these places, the only reason the source is created is to get to the root_package. This just extracts the source creation into a Package constructor for now, but I think this can be made to not use source at all.
As alluded to in the previous commit, we don't actually need a Source at all in order to be able to get a Package given a manifest and a config.
So registry preloading was added[1] to avoid updating the root path multiple times unnecessarily, since that's an expensive operation. But with Package::from_path, we can get the root package just from a manifest and a config without having to update the whole path source. So now we don't have to remember to preload the registry if we've already updated a source and want to use the registry later. Instead, we just avoid loading the source initially and let the registry do it when it needs to -- which is now in resolve_with_previous. This does cause one tested error message to happen slightly later than it used to, but it still happens. [1] - ef8c651
This code was happening in both cargo fetch and cargo compile and had a slightly different error message in each ;)
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
bors
added a commit
that referenced
this pull request
Sep 20, 2015
Hiii! These are just a few small cleanup things that I took care of while working on #1991, but these should all be less controversial than that one is :)
| let resolve = try!(ops::resolve_pkg(&mut registry, &package)); | ||
| let _ = get_resolved_packages(&resolve, &mut registry); |
There was a problem hiding this comment.
This looks like it should be wrapped in a try!, right?
|
Nice! I really like this method of deferring the update of a path package until it's necessary. Just a few nits and otherwise r=me |
If at first you don't succeed to call try!, try try! again ;)
|
Whew, I'm glad you like it!! I added 2 new commits for the changes; I'm happy to squash and rebase if you'd like. |
bors
added a commit
that referenced
this pull request
Sep 22, 2015
Hi! This is an attempt to start refactoring some of the internals to be more like a pipeline, and eventually enable the kind of functionality I tried to add in #1968 without having to add as much duplication. Turns out there's a fair bit of duplication in the code today, I think this helps address it! I may have totally gone against some abstractions... namely I made a way to create `Package`s from a `manifest_path` and a `config`, without needing a `Source`. I think it cleans up the code quite a bit, and I think makes things a bit more pipeliney in that the `Source` isn't updated until we really need it to be (as opposed to having to use `preload` to avoid updating it again). But I'm open to the possibility that I'm moving things around to where no one who knows the code well will be able to find them ;) This *should* be a Real Refactor in the sense that these changes don't change behavior-- except in one test case, where the same error happens as did before, but it's going through a `chain_error` now so has a slightly different message.
|
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
This was referenced Sep 22, 2015
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi! This is an attempt to start refactoring some of the internals to be more like a pipeline, and eventually enable the kind of functionality I tried to add in #1968 without having to add as much duplication. Turns out there's a fair bit of duplication in the code today, I think this helps address it!
I may have totally gone against some abstractions... namely I made a way to create
Packages from amanifest_pathand aconfig, without needing aSource. I think it cleans up the code quite a bit, and I think makes things a bit more pipeliney in that theSourceisn't updated until we really need it to be (as opposed to having to usepreloadto avoid updating it again). But I'm open to the possibility that I'm moving things around to where no one who knows the code well will be able to find them ;)This should be a Real Refactor in the sense that these changes don't change behavior-- except in one test case, where the same error happens as did before, but it's going through a
chain_errornow so has a slightly different message.