Find cargo toml up the fs #3309
Conversation
… into find-cargo-toml-up-the-fs
|
Thanks. This seems like a reasonable idea, but I have a small nit: it doesn't match my usual idea of going up (towards the root) or down (into a subdirectory) the file system 😄. |
|
You are right, I mixed up the terminology (thinking roots of a tree are down in the ground 😄 ). It is offically the reverse https://en.wikipedia.org/wiki/Cd_(command)
Also having slept on it I think I should inlude logic to collect all Cargo.toml-s in the sub dirs and if there are multiple options to abandon the process? This will solve weird cases where otherwise users would experience that some stuff is working and other stuff isn't because one sub Cargo.toml is used. To be throrough and pendantic an alternative error to the |
|
Yeah, I think it's a good idea to error out instead of picking a random |
| if let Some(p) = find_cargo_toml_up_the_fs(path) { | ||
| return Ok(p) | ||
| } | ||
|
|
||
| Err(CargoTomlNotFoundError(path.to_path_buf()).into()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah, the naming is confusing... Let's do find_in_parent_dir, find_in_child_dir mabye?
There was a problem hiding this comment.
I think you might have meant this anyway but just mentioning that I prefixed with find_cargo_toml_... because these functions are in a fairly "large" file so a new reader might wonder - find what?
crates/ra_project_model/src/lib.rs
Outdated
|
|
||
| impl Error for CargoTomlNoneFoundError {} | ||
| impl Error for CargoTomlMultipleValidFoundError {} | ||
| impl Error for CargoTomlSearchFileSystemError {} |
There was a problem hiding this comment.
Hope this method of grouping is acceptable? Group related structs and impls together.
I don't have a strong preference, I can more them around if that is preferred. :)
crates/ra_project_model/src/lib.rs
Outdated
| pub struct CargoTomlMultipleValidFoundError(pub Vec<PathBuf>); | ||
|
|
||
| #[derive(Clone, PartialEq, Eq, Hash, Debug)] | ||
| pub struct CargoTomlSearchFileSystemError(pub PathBuf, pub String); |
There was a problem hiding this comment.
I went this route to enumerate the possibilities, could also have been one error like:
pub enum CargoTomlFindErrorType {
NoneFound
MultipleValidFound
FilesystemError
}
pub struct CargoTomlFindError(pub PathBuf, typ CargoTomlFindErrorType)
However I don't THINK this is "rusty" or a common practice in general nor in this codebase?
There was a problem hiding this comment.
Let's stick with a single error type here
struct CargoTomlNotFoundError {
searched_at: PathBuf,
reason: String,
}
Regarding last changesI made the solution a bit more complicated to pass the details of what happened to the user, thinking it might be worth it. TestingCovered trying these cases by using the rust-analyzer codebase: [x] Without changes -> all ~200 packages loaded [x] Rename Cargo.toml in root to _Cargo.toml -> xtask Cargo.toml is found [x] Rename in root & Add a fake one to crates -> duplicate error
[x] Rename in root & simulate a fs error -> fs error
|
|
bors r+ |
Build succeeded |
The rust-analyzer plugin for VS Code cannot discover our Cargo.toml because it is two levels below the root directory, and rust-analyzer only searches one level deep by default. (See rust-lang/rust-analyzer#3309) To remedy this, let's add a per-project configuration file for VS Code that specifies the path to Cargo.toml.
The rust-analyzer plugin for VS Code cannot discover our Cargo.toml because it is two levels below the root directory, and rust-analyzer only searches one level deep by default. (See rust-lang/rust-analyzer#3309) To remedy this, let's add a per-project configuration file for VS Code that specifies the path to Cargo.toml.
Currently rust-analyzer will look for Cargo.toml in the root of the project and if failing that then go down the filesystem until root.
This unfortunately wouldn't work automatically with (what I imagine is) a fairly common project structure. As an example with multiple languages like:
Added this small change so rust-analyzer would glance one level up if not found in root or down the filesystem.
Why not go deeper?
Could be problematic with large project vendored dependencies etc.
Why not add a Cargo.toml manual setting option?
Loosely related and a good idea, however the convenience of having this automated also is hard to pass up.
Testing?
Build a binary with various logs and checked it in a project with such a structure:
Edge Cases?
If you have multiple Cargo.toml files one level deeper AND not in the root, will get whatever comes first (order undefined), example:
However this is quite unusual and wouldn't have worked before either. This is only resolvable via manually choosing.