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

Conversation

@Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Dec 30, 2017

Fixes #179.

This allows the 'rust-client.channel' option to be null and if so, the extension tries to infer the active channel from running rustup show in current workspace directory.

Manually tested with alternating stable/nightly toolchain and also with a toolchain override (servo repo), but it'd be good to also include a test for that.

r? @nrc

/**
* Parses given output of `rustup show` and retrieves the local active toolchain.
*/
export function parseActiveToolchain(rustupOutput: string): string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't async, since it needs to finish to return a value with which global RLSConfiguration is created.
For the same reason it only parses the result and it's not a complete function that uses CONFIGURATION.rustupPath value, since it's called during CONFIGURATION construction.

As a side note, the configuration probably will need to be redesigned later on, as currently it's a global that's lazily constructed once and doesn't handle 'rust-client.*' value changes

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 add the 'side note' as a FIXME or file an issue please?

/**
* Parses given output of `rustup show` and retrieves the local active toolchain.
*/
export function parseActiveToolchain(rustupOutput: string): string {
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 add the 'side note' as a FIXME or file an issue please?

* Tries to fetch the `rust-client.channel` configuration value. If it's null,
* then it tries to query active channel using rustup given at `rustupPath`.
*/
private static readChannel(rustupPath: string, configuration: WorkspaceConfiguration): string {
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 whole function should live in rustup.ts, rather than here.

@Xanewok Xanewok force-pushed the default-user-toolchain branch from 7cbc5ca to be6e9fb Compare January 13, 2018 22:38
@Xanewok
Copy link
Contributor Author

Xanewok commented Jan 13, 2018

Done! Sorry it took so long, I forgot about it 😞

Can't think of a way to reliably test it (especially detecting local overrides) apart from using docker or a separate bash script that sets default toolchain to different one and tests the function output somehow with each change.

@nrc nrc merged commit faa0343 into rust-lang:master Jan 15, 2018
@nrc
Copy link
Member

nrc commented Jan 15, 2018

Thanks!

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