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

search for the cargo toml based on the current file #457

Merged

Conversation

jannickj
Copy link
Contributor

Code for the proposed fix: #222

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! All looks good, just a query about giving a warning when we can't find a Cargo.toml

@@ -53,6 +54,10 @@ function didOpenTextDocument(document: TextDocument, context: ExtensionContext):
return;
}
folder = getOuterMostWorkspaceFolder(folder);
folder = getCargoTomlWorkspace(folder, document.uri.fsPath);
if (!folder) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Can/should we issue a warning about missing Cargo.toml here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes probably a good idea

Copy link
Contributor Author

@jannickj jannickj Dec 5, 2018

Choose a reason for hiding this comment

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

My only concern is that you will get a pop up if you are just opening a .rs file outside it's project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I've come up with a solution that will work in both use cases.

@jannickj
Copy link
Contributor Author

jannickj commented Dec 5, 2018

Changed how the message is shown from a pop up to a nice little message in the bottom, much more pleasant from a UX perspective.

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.

Great, thanks!

@nrc nrc merged commit e414e6a into rust-lang:master Dec 5, 2018
@maelvls
Copy link
Contributor

maelvls commented Dec 5, 2018

Thank you, @jannickj!!! 🎉 🍾

@rickvanprim
Copy link

@nrc @jannickj I see this changed was backed out. Any context on why? Will this feature be re-added soon?

@Xanewok
Copy link
Member

Xanewok commented Mar 13, 2019

IIRC this introduced a bug that broke project detection and it did impact a lot of users.

Could you remind me what this aims to fix? Not warning against missing Cargo.toml when the user opens a project that has a Rust project nested and not at the top level?

I'll try to tackle this again in the immediate future.

@rickvanprim
Copy link

I can't speak for others, but for me this being able to have a project that contains one or more Rust crates mixed in with other projects, where Cargo is not the root build system and Rust is not the only language. RLS detecting Cargo.toml relative to the source file being looked at means the crate can be located anywhere in the repo, not just at the root.

@rickvanprim
Copy link

@Xanewok any updates?

@jannickj
Copy link
Contributor Author

jannickj commented Aug 6, 2019

@Xanewok The problem is with having multiple rust project in the space workspace.

The current setup is that the Cargo.toml must be in the root of the workspace. However with this change it would allow the project to work in any subfolder. Furthermore it would spin up multiple RLS instances when there were multiple Cargo.toml

@rickvanprim I remember the problem being impact on windows users. I use linux exclusively and hadn't properly tested nor forwarned that it probably should be tested.

I will see if I can fix the compatibility problems, and then try again and hopefully we can have others help with the testing.

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.

Incorrectly reports missing Cargo.toml if it's not in the root
5 participants