Rewrite the cargo fix command
#66
Conversation
|
I gave this a spin with |
This commit starts rewritign the `cargo fix` command in line with the recent discussions around the edition. The main changes here are: * The interactivity of `cargo fix` is removed * Executing `cargo fix` is now a thin veneer over `cargo check` * Fixes are automatically applied when it looks very likely to succeed if we apply them. * Fixes are only applied to the local workspace rather than upstream crates as well. There's still a number of missing features to fill out marked with a few TODO here and there but the test suite is already looking relatively solid. I think we've got a long way to go here but this is hopefully a good start in the direction of where we want to go.
alexcrichton
force-pushed
the
rewrite-cargo-fix
branch
from
be6c023 to
400ae0e
Compare
There was a problem hiding this comment.
Very nice! I've certainly had fun reading this on my way to the office today! I've mentioned a few things below, and pushed two small commits to add a comment and simplify some .context calls.
I'll try to play around with this a bit more, but I'm good with landing this as-is and opening follow-up PRs.
| failure = "0.1" | ||
| rustfix = { path = "..", version = "0.2" } | ||
| serde_json = "1" | ||
| log = "0.4" |
There was a problem hiding this comment.
We should probably also add env_logger or something like it to actually print the logs
| serde_derive = "1.0" | ||
| rustfix = "0.2.0" | ||
| failure = "0.1" | ||
| rustfix = { path = "..", version = "0.2" } |
There was a problem hiding this comment.
Why is this better than the [patch] in the workspace manifest? (If the lib was a subdir with its own manifest, using patch we could cargo publish without manually changing toml files beforehand)
There was a problem hiding this comment.
Ah this is actually drawing a dependency there and is sort of more robust in terms of workspace membership and speed of resolution. Using path guarantees that you'll use the rustfix above, whereas with [patch] you're just "pretty likely to"
| impl<'a> Drop for ExpectCmd<'a> { | ||
| fn drop(&mut self) { | ||
| if !self.ran { | ||
| panic!("forgot to run this command"); |
There was a problem hiding this comment.
Heh, interest way to write #[must_use] and session types ;)
There was a problem hiding this comment.
True! This has worked out pretty well in Cargo as it's not absolutely crucial to nail ergonomcis but rather just catch accidental mistakes here and there
| me.push("generated-tests"); | ||
| me.push(&format!("test{}", idx)); | ||
| return me | ||
| } |
There was a problem hiding this comment.
This whole fn looks like a tempdir::TempDir::new() to me. The thread_local index that on init increments an atomic is pretty cool though (took me a while to see what's going on). Still haven't seen why the project dirs ever get cleaned up.
There was a problem hiding this comment.
Indeed! I learned this when working on other CLI tools that the achilles heel of TempDir is the reason it exists, automatic cleanup. When debugging why a test fails it's often difficult and annoying to set up the workspace again and it's often much easier to just go poking around directly in the target directory to see what went wrong
| println!("\nDone."); | ||
| let new_code = rustfix::apply_suggestions(&code, &suggestions); | ||
| File::create(&file) | ||
| .and_then(|mut f| f.write_all(new_code.as_bytes())) |
There was a problem hiding this comment.
Oh, do we need to truncate the file or does create + write_all do that automatically?
There was a problem hiding this comment.
Ah File::create automatically truncates a file that it opens, it's only OpenOptions::new().write(true) that won't do that.
|
Ok! I'm gonna land this and we can continue to iterate in-tree |
This commit starts rewritign the
cargo fixcommand in line with the recentdiscussions around the edition. The main changes here are:
cargo fixis removedcargo fixis now a thin veneer overcargo checkapply them.
well.
There's still a number of missing features to fill out marked with a few TODO
here and there but the test suite is already looking relatively solid. I think
we've got a long way to go here but this is hopefully a good start in the
direction of where we want to go.