Skip to content

Renames #1237

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 4 commits into from
Aug 28, 2017
Merged

Renames #1237

merged 4 commits into from
Aug 28, 2017

Conversation

nrc
Copy link
Member

@nrc nrc commented Aug 17, 2017

This adds a renames section to the manifest. The idea is that we will add a rename from rls to rls-preview at the same time as renaming the rls component to rls-preview. This new feature will cause all existing users of the rls to have that component renamed to rls-preview. When the rls hits 1.0 we'll change the rename from rls-preview to rls and then everyone who already has the rls will be moved to the 1.0 name. Likewise for rustfmt, etc.

r? @alexcrichton

@@ -118,6 +123,28 @@ impl Manifest {
result
}

fn table_to_renames(mut table: toml::Table, path: &str) -> Result<HashMap<String, String>> {
let mut result = HashMap::new();
let rename_table = try!(get_table(&mut table, "rename", path));
Copy link
Member

Choose a reason for hiding this comment

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

I think this means that this'll fail if rename isn't present, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, if there is no table get_table returns Ok(toml::Table::new())

Copy link
Member

Choose a reason for hiding this comment

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

Ok great!


for (k, v) in rename_table {
if let toml::Value::Table(mut t) = v {
result.insert(k.to_owned(), get_string(&mut t, "to", path)?);
Copy link
Member

Choose a reason for hiding this comment

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

How come the level of indirection here? Why not:

[rename]
cargo-old = 'cargo'

(etc)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was basically following the convention for pkg, I also thought it was a little bit more clear, but I'm not attached to the current format, if you'd prefer to change.

Copy link
Member

Choose a reason for hiding this comment

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

Ok sounds good to me!

@alexcrichton
Copy link
Member

Looking good! Could this also add a tests/*.rs style integration test which runs through the rustup commands themselves to ensure that all works?

The two cases I'm thinking about are:

  • First a manifest only has rls-preview, you install it, then we publish a manifest with a [rename] that doesn't have rls-preview, and your rustup update should work.
  • Second a manifest has a [rename] and adding the renamed component should work

@nrc
Copy link
Member Author

nrc commented Aug 25, 2017

Tests added!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 25, 2017

📌 Commit dea6e4b has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 25, 2017

⌛ Testing commit dea6e4b with merge 1392c81...

bors added a commit that referenced this pull request Aug 25, 2017
Renames

This adds a `renames` section to the manifest. The idea is that we will add a rename from `rls` to `rls-preview` at the same time as renaming the rls component to rls-preview. This new feature will cause all existing users of the rls to have that component renamed to rls-preview. When the rls hits 1.0 we'll change the rename from `rls-preview` to `rls` and then everyone who already has the rls will be moved to the 1.0 name. Likewise for rustfmt, etc.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Aug 25, 2017

💔 Test failed - status-travis

@nrc
Copy link
Member Author

nrc commented Aug 27, 2017

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 27, 2017

@nrc: 🔑 Insufficient privileges: Not in reviewers

@nrc
Copy link
Member Author

nrc commented Aug 27, 2017

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 27, 2017

@nrc: 🔑 Insufficient privileges: Not in reviewers

@Diggsey
Copy link
Contributor

Diggsey commented Aug 27, 2017

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 27, 2017

📌 Commit 2f02956 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 27, 2017

⌛ Testing commit 2f02956 with merge 14fb13c...

bors added a commit that referenced this pull request Aug 27, 2017
Renames

This adds a `renames` section to the manifest. The idea is that we will add a rename from `rls` to `rls-preview` at the same time as renaming the rls component to rls-preview. This new feature will cause all existing users of the rls to have that component renamed to rls-preview. When the rls hits 1.0 we'll change the rename from `rls-preview` to `rls` and then everyone who already has the rls will be moved to the 1.0 name. Likewise for rustfmt, etc.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Aug 27, 2017

💔 Test failed - status-travis

@nrc
Copy link
Member Author

nrc commented Aug 28, 2017

@bors: retry

@bors
Copy link
Contributor

bors commented Aug 28, 2017

@nrc: 🔑 Insufficient privileges: and not in try users

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Aug 28, 2017

⌛ Testing commit 2f02956 with merge 07fd5bf...

bors added a commit that referenced this pull request Aug 28, 2017
Renames

This adds a `renames` section to the manifest. The idea is that we will add a rename from `rls` to `rls-preview` at the same time as renaming the rls component to rls-preview. This new feature will cause all existing users of the rls to have that component renamed to rls-preview. When the rls hits 1.0 we'll change the rename from `rls-preview` to `rls` and then everyone who already has the rls will be moved to the 1.0 name. Likewise for rustfmt, etc.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Aug 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 07fd5bf to master...

@bors bors merged commit 2f02956 into rust-lang:master Aug 28, 2017
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.

4 participants