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

Added custom path for rustup #161

Merged
merged 1 commit into from
Oct 9, 2017
Merged

Added custom path for rustup #161

merged 1 commit into from
Oct 9, 2017

Conversation

yisonPylkita
Copy link
Contributor

Closes #160

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 PR and sorry it has taken a while to review - I am on parental leave at the moment and have terrible internet, so it's taking me a while.

The PR looks good; I left a few comments inline to address.

@@ -44,14 +45,15 @@ export class RLSConfiguration {
* directory.
*/
public readonly rlsRoot: string | null;

Copy link
Member

Choose a reason for hiding this comment

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

nit: please don't add whitespace

Copy link
Contributor Author

@yisonPylkita yisonPylkita Oct 5, 2017

Choose a reason for hiding this comment

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

Ok, I will correct this and pay attention to this in future.

Thanks for pointing this out.

public static loadFromWorkspace(): RLSConfiguration {
const configuration = workspace.getConfiguration();

return new RLSConfiguration(configuration);
}

private constructor(configuration: WorkspaceConfiguration) {
this.rustupPath = configuration.get('rustup.path', 'rustup');
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 use something likerust-client.rustupPath for the configuration name please? Could you also add the option to the configuration section of package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you are right.

I will submit patch when I get home.

@nrc
Copy link
Member

nrc commented Oct 7, 2017

Looks good, thanks! Could you rebase please?

@yisonPylkita
Copy link
Contributor Author

Sorry for the delay.
I have rebased this branch onto actual master.

@Xanewok Xanewok merged commit c47d0a7 into rust-lang:master Oct 9, 2017
@Xanewok
Copy link
Member

Xanewok commented Oct 9, 2017

Thank you!

@yisonPylkita yisonPylkita mentioned this pull request Jun 12, 2018
Xanewok pushed a commit to Xanewok/rls-vscode that referenced this pull request Mar 27, 2019
…ng errors. (rust-lang#164)

* Fixed erasing of a document on reformatting if the document has parsing errors.
Fixed rust-lang#161.

* Changed the condition checked to determine whether there are errors.

* Removed the second part of the comment.
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.

3 participants