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

Resolve potential future shock (x.yyy.zz) #2132

Merged
merged 2 commits into from Dec 3, 2019

Conversation

@kinnison
Copy link
Collaborator

kinnison commented Nov 17, 2019

This is intended to fix #2129 which suggests we have seven years until the version'pocalypse.

cc @carols10cents @Diggsey

kinnison added 2 commits Nov 17, 2019
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
To simplify the amount of code and places where we parse toolchain
channel names, make the channels and regexp global.  Also support
version numbers of the form up to x.yyy.zz in order that we can
future-proof ourselves.

Fixes #2129

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@kinnison kinnison added this to the 1.21.0 milestone Nov 17, 2019
r"^({})(?:-(\d{{4}}-\d{{2}}-\d{{2}}))?(?:-(.*))?$",
TOOLCHAIN_CHANNELS.join("|")
);
// Note this regex gives you a guaranteed match of the channel (1)

This comment has been minimized.

Copy link
@Diggsey

Diggsey Nov 18, 2019

Contributor

Instead of writing this in a comment and referring to numeric capture group IDs, we could encode this in the type system, eg.

struct ParsedToolchainDesc {
    channel: String,
    date: Option<String>,
    target: Option<String>,
}

fn parse_toolchain_desc(input: &str) -> Result<ParsedToolchainDesc, Error> {
    ... local regex and extraction code ...
}

... and then this function could be used by both FromStr implementations. It would also make it easy to unit test the parsing function.

This comment has been minimized.

Copy link
@kinnison

kinnison Nov 18, 2019

Author Collaborator

You are quite right, and have seen through to my transparent attempt to leave nice refactoring jobs open for newcomers to the project. If you'd rather I do this myself then I'm happy to, otherwise my intent was to file an issue describing something (perhaps in less concrete terms than your comment) to encourage someone to come and make a niceness happen as a way into the codebase. In your view, should I make the change, or should I be filing that issue? I'm happy either way.

This comment has been minimized.

Copy link
@kinnison

kinnison Dec 3, 2019

Author Collaborator

After two weeks I filed #2142

@kinnison kinnison merged commit 5f4d2dc into rust-lang:master Dec 3, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kinnison kinnison deleted the kinnison:kinnison/future-shock branch Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.