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

Skip Scanning Index with Exact Version #228

Merged
merged 8 commits into from
Jan 4, 2019

Conversation

charlespierce
Copy link
Contributor

Closes #227

Prevent an unnecessary scan of the tool index (either Node or Yarn) by adding a VersionSpec::Exact type to represent a known exact version. Then when resolving the version we can shortcut and just return that version instead of doing a scan.

Also updated the error messages in distro to handle the 404 response, which will most often occur when an exact version that doesn't exist is specified. Note: I'm especially interested in feedback on the approach taken to refactor the error messages, as I feel like there could be a more elegant way to approach the problem.

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This looks good to me -- I'm not sure what in particular was bothering you about the error message logic. I pointed out a couple minor style points but I didn't see anything really wrong!

I'll leave it up to you if you want to make any of the tiny changes I suggested.

version: version,
from_url: from_url,
error: error.to_string(),
move |error| match error.downcast_ref::<HttpError>() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might separate the match to a new line and brace the closure body:

move |error| {
    match error.downcast_ref::<HttpError>() {
        // ...
    }
}

But otherwise this looks fine to me.

from_url: from_url,
error: error.to_string(),
move |error| match error.downcast_ref::<HttpError>() {
Some(HttpError { code }) if *code == StatusCode::NotFound => DownloadError::NotFound {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is what was bothering you, it's fine. :) I'd probably prefer if-let but you can't concisely check the status code without the if-let-chains feature, which hasn't been implemented yet.

use reqwest::StatusCode;
use std::fmt;

// Once Issue #173 is implemented, we can use the ToolSpec struct to differentiate tools
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to use a standard syntax for issue comments, esp for greppability. We've been using:

// ISSUE #173: Once it's implemented we can use the ToolSpec struct etc etc

@charlespierce
Copy link
Contributor Author

Updated with the style changes. There wasn't anything specific bothering me about it, just mostly looking for a sanity check that there wasn't a simpler way that I was overlooking :)

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

Beauty :)

@charlespierce
Copy link
Contributor Author

charlespierce commented Jan 4, 2019

Updated the FromStr implementation for VersionSpec to first attempt to parse an exact Version from the string, if that succeeds then we get a VersionSpec::Exact instance, otherwise we parse the string as a Version Requirement and get a VersionSpec::SemVer.

This caused a few other changes as other tests were failing, so also updated the exit code for a Distro::DownloadError to better match the previous implementation, and set up the test sandbox to support 404 responses.

This has the upside of making notion pin yarn 1.13.0 and notion install yarn 1.13.0 bypass needing to download the version index, as we can parse it directly to an exact version (Though notion pin yarn 1 will still correctly download the index and look for a match to the SemVer requirement 1).

@dherman
Copy link
Collaborator

dherman commented Jan 4, 2019

@charlespierce This looks good. It's kind of a shame semver::VersionReq doesn't provide the ability to test for exact versions and extract them as semver::Versions, since this means we end up double-parsing a bit. But these are very small strings so it doesn't really matter.

@dherman dherman merged commit af4bfb6 into volta-cli:master Jan 4, 2019
@charlespierce charlespierce deleted the exact_version_scan branch January 7, 2019 18:27
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.

2 participants