Skip to content
This repository was archived by the owner on Nov 18, 2022. It is now read-only.

Use the user's default toolchain #180

Closed
wants to merge 1 commit into from
Closed

Use the user's default toolchain #180

wants to merge 1 commit into from

Conversation

lvillani
Copy link

Since Rust 1.21 the RLS can be installed from the stable channel.

Switching to use the stable channel by default would let people who
prefer to depend only on the stable compiler to have nightly installed
for one less thing.

@nrc
Copy link
Member

nrc commented Oct 27, 2017

In #179 I propose a slightly different approach - using the current Rustup default as the default. So if the user is using stable, they'll get stable and if they're using nightly they'll get nightly. I'd prefer not to always use stable as default since for users with both nightly and stable toolchains, they'll get a significantly better experience with nightly.

@lvillani
Copy link
Author

Ah! That makes total sense. I'll try to update this PR later this week.

@lvillani lvillani changed the title Default to 'stable' channel Use the user's default toolchain Nov 2, 2017
@lvillani
Copy link
Author

lvillani commented Nov 2, 2017

I changed the PR to look up for the default toolchain. Not sure how it fits with rust-lang/rustup#1279 (comment), though. Also not sure why the build on Travis is failing, it builds just fine for me.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. I think by moving the default handling code to rustup.rs you'll be able to share some code with the existing functions there.

@@ -170,7 +170,7 @@
},
"rust-client.channel": {
"type": "string",
"default": "nightly",
"default": "default",
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 you can use null rather than "default"

} catch (e) {
console.log(e);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this code to rustup.rs?

Copy link
Author

@lvillani lvillani Nov 11, 2017

Choose a reason for hiding this comment

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

I thought about moving it there but most of the code in rustup.ts relies on the RLSConfiguration object being already initialised, and is async. I thought this code would be inconsistent with the rest of the code there. I have no problem moving it there though.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're using RLSConfiguration here too though? I don't think that whether the code is sync or async should make too much difference as to where it should live.

@nrc
Copy link
Member

nrc commented Nov 5, 2017

Not sure why Travis is failing. Looks like a TypeScript compilation error, but it's not in a place that I would have thought your code should affect.

@nrc
Copy link
Member

nrc commented Nov 15, 2017

The test failure is weird - I see it locally too, but that code hasn't changed for months. I guess a library changed or something. Anyway, not your problem :-)

Since Rust 1.21 the RLS can be installed from the stable channel, which
means that most new users can install the RLS with the toolchain they
get by default via rustup.
@lvillani
Copy link
Author

While working on this I found several edge cases and I have not enough time to handle them at the moment. I'm closing this PR for now.

@lvillani lvillani closed this Dec 26, 2017
Xanewok pushed a commit to Xanewok/rls-vscode that referenced this pull request Mar 27, 2019
* some clippy-suggested improvements

* match instead of if, pattern simplification
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants