Add support for new path dep field #149
Conversation
|
rust-lang/cargo#8994 was just accepted and is being merged. Will land in Rust 1.51 if all goes according to plan. |
|
Is it reasonable to merge this before 1.51 is released, given that it's documented that the field will be |
|
Yes, seems fine to me to merge this, we just need to change the tests in CI to make sure they work right now |
|
@oli-obk I updated the test to only test for a |
tests/test_samples.rs
Outdated
| path_dep | ||
| .path | ||
| .as_ref() | ||
| .map(|p| p.ends_with("path-dep/Cargo.toml")), |
There was a problem hiding this comment.
I don't think the path ends with Cargo.toml, you may want something more like this:
assert_eq!(
path_dep.path.as_ref().unwrap().file_name().unwrap().to_str().unwrap(),
"path-dep",
);There was a problem hiding this comment.
Ah, yes, you're right, I forgot to update the path following the change from manifest_path to path upstream. Will fix shortly!
There was a problem hiding this comment.
Also, the reason I did not assert the full string was that I believe it will be an absolute path, not a relative path. Is that not the case?
There was a problem hiding this comment.
It's an absolute path. The snippet I posted calls file_name just to get the last part. You could also check if it ends with tests/all/path-dep.
There was a problem hiding this comment.
Ah, missed the call to file_name. I think ends_with("path-dep") is probably true enough to what we want to test, without the potential of generating false-positives if the test directories get moved around. If you'd strongly prefer the file_name() or a longer prefix for ends_with though, I'd be happy to update the PR.
|
I think this is now ready to merge and release? |
|
Jup! Thanks for the ping |
This is pending for when rust-lang/cargo#8994 eventually lands, and would be very useful for rust-lang/rustfmt#4247 (comment).