Use a more clever way to replace parts of a file #67
Conversation
|
Works great on top of #66 after adding a single |
killercup
force-pushed
the
cool-replacements
branch
from
df52ebe
to
7a987f1
Compare
|
Nice! I think that merging the |
|
Also you may want to rebase on master now as it turns out I forgot to include running of the new tests in the branch this is based off! |
killercup
force-pushed
the
cool-replacements
branch
from
7a987f1
to
c517874
Compare
|
I think this'll handle issues like #77, right? If so, could that test be included? |
Cargo.toml
Outdated
| @@ -20,13 +20,16 @@ exclude = [ | |||
| serde = "1.0" | |||
| serde_json = "1.0" | |||
| serde_derive = "1.0" | |||
| replace-rs = { git = "https://github.com/killercup/replace-rs" } | |||
There was a problem hiding this comment.
This can be removed now I think
src/lib.rs
Outdated
| @@ -208,16 +219,22 @@ pub fn apply_suggestion(file_content: &mut String, suggestion: &Replacement) -> | |||
| new_content | |||
There was a problem hiding this comment.
Should this method be removed now?
There was a problem hiding this comment.
Yep. Breaking change! 🙌
|
|
||
| proptest! { | ||
| #[test] | ||
| #[ignore] |
There was a problem hiding this comment.
How come these are #[ignore]? Do they take too long?
There was a problem hiding this comment.
They take a few seconds each, yeah. I don't really want to run them on each cargo test, but I'll add them to CI
|
Moved it in as a module, and added tests from #77 which automagically work now. I've also added another test for "overlapping" suggestions. The only one I could think of that I know exists in rustc is missing imports with multiple options. If you have an idea for other tests, let me know (or just add them :)) |
|
|
||
| let stderr = "\ | ||
| [CHECKING] foo v0.1.0 (CWD) | ||
| error: Cannot replace slice of data that was already replaced |
There was a problem hiding this comment.
Hm I'm not sure that this is the UI we're gonna want long-term, I think we probably want to just skip auto-fixing code and let warnings like these naturally bubble up to the user?
Could we add some logic to avoid attempting to apply suggestions that look like they're gonna fail? (or otherwise just keep track of suggestions that fail to apply)
There was a problem hiding this comment.
Yes that's why I was asking for a better test case :) I opened #78 for the intended user experience
There was a problem hiding this comment.
Hm could we tweak this test to assert that the warning isn't updated here? I think it's probably most conservative to start out by not applying any suggestions when they're ambiguous about what to do. Anything we don't fix will end up being seen by the end user anyway
There was a problem hiding this comment.
Yeah. Do you want me to do this here or in another PR? (The behavior isn't changed here AFAIK)
There was a problem hiding this comment.
Ah ok let's do that in a follow-up then
|
|
||
| fn compile(file: &Path) -> Result<Output, Box<Error>> { | ||
| let tmp = TempDir::new("rustfix-tests")?; | ||
| let better_call_clippy = cmd!( | ||
| "rustc", file, | ||
| "--error-format=pretty-json", "-Zunstable-options", "--emit=metadata", | ||
| "--crate-name=rustfix_test", | ||
| "-Zsuggestion-applicability", |
There was a problem hiding this comment.
Oh! Don't look! Spoilers!
alexcrichton
force-pushed
the
cool-replacements
branch
from
3ceae9c
to
f9b8801
Compare
Addresses #57
Btw, this is still only concerning itself with replacing multiple things in one file. #66 changes the CLI to group suggestions by file. Not sure if we want to make this part of the library.