Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support EmitMode::ModifiedLines with stdin input #3424

Merged
merged 5 commits into from
Mar 5, 2019

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Mar 1, 2019

Main motivation is to handle the case where we have an input from stdin and we're mostly interested in a diff rather than whole file and trying to come up with this information ourselves. The EmitMode::ModifiedLines should still support writing to a specified Write sink - we mostly lacked the information on what was the input text so we ask that the passed source_map.

Currently works with https://github.com/Xanewok/rls/tree/rustfmt-modified-lines branch on the RLS side.

related #1173 (although this still has to go through (de)serialization)

@Xanewok
Copy link
Member Author

Xanewok commented Mar 1, 2019

CI fails on clippy failure, which in turn is blocked on Manishearth/compiletest-rs#160

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The implementation looks good, though the new test is failing on Windows, possibly due to \n and \r\n things. Would you mind fixing it?

fn from(mismatches: Vec<Mismatch>) -> ModifiedLines {
let chunks = mismatches.into_iter().map(|mismatch| {
let lines = || mismatch.lines.iter();
let num_removed = lines()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using a closure here? Is this for some kind of optimization? Just curious...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the closure here is redundant; I'd personally just do all of that as chained expressions but such a conceptually simple things blows to ~10 lines 😢 So I just tried to split this into two to appease Rustfmt; I'll try with the version without a closure.

Xanewok added 5 commits March 4, 2019 18:19
This moves `Modified{Chunks,Lines}` from `src/formatting.rs` to
`src/rustfmt_diff.rs` and reexports it in `src/lib.rs`.

With this, a conversion from `Vec<Mismatch>` to `ModifiedLines` was implemented
and now this implements complementary `Display` and `FromStr`, which
simplified the previously used `output_modified` function and which allows to
parse the raw data emitted with `EmitMode::ModifiedLines`.
@Xanewok Xanewok force-pushed the modified-lines-for-stdin branch from 42aa5a1 to fbfda61 Compare March 4, 2019 18:43
@Xanewok
Copy link
Member Author

Xanewok commented Mar 4, 2019

CI failed on ICE in futures-rs:

error: internal compiler error: src/librustc_codegen_llvm/debuginfo/metadata.rs:661: debuginfo: unexpected type in type_metadata: impl core::future::future::Future

@scampi
Copy link
Contributor

scampi commented Mar 4, 2019

@Xanewok looks like there's a PR out already: rust-lang/rust#58888

@topecongiro topecongiro merged commit 06fc39f into rust-lang:master Mar 5, 2019
@topecongiro
Copy link
Contributor

LGTM, thanks for the update!

@Xanewok
Copy link
Member Author

Xanewok commented Mar 5, 2019

Thank you! Do you think you could publish a point release with this or do you prefer to accumulate more changes for the release, first?

@Xanewok Xanewok deleted the modified-lines-for-stdin branch March 5, 2019 11:59
bors added a commit to rust-lang/rls that referenced this pull request Apr 6, 2019
Return only modified lines in formatting responses

Ticks few boxes in #3. (that's an early issue!)
Closes #334.

This is currently based on rust-lang/rustfmt#3424 and so currently blocked until it merges.

Configures Rustfmt to use `EmitMode::ModifiedLines` and parses the response - this should work with both statically-linked and externally provided Rustfmt.

@alexheretic do you think you could take a look and double-check?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants