-
Notifications
You must be signed in to change notification settings - Fork 164
search for the cargo toml based on the current file #457
search for the cargo toml based on the current file #457
Conversation
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
Thank you, @jannickj!!! 🎉 🍾 |
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. |
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. |
@Xanewok any updates? |
@Xanewok The problem is with having multiple rust project in the space workspace. The current setup is that the @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. |
Code for the proposed fix: #222