Backport #4524 and fix #5138 - proper wrap strings with escape backslash #5483
Conversation
…with escape backslash
src/string.rs
Outdated
| // Ensure break is not after an escape '\' as it will "escape" the `\` that is added | ||
| // for concatenating the two parts of the broken line. |
There was a problem hiding this comment.
I think this rephrasing could help explain that we're trying not to escape any line continuation characters that we add when we break the string.
| // Ensure break is not after an escape '\' as it will "escape" the `\` that is added | |
| // for concatenating the two parts of the broken line. | |
| // Ensure break is not after an escape '\' as it will "escape" the line continuation `\` | |
| // that is added for concatenating the two parts of the broken line. |
There was a problem hiding this comment.
Comment enhanced in line with the requested change.
src/string.rs
Outdated
| // There is a non-`\` to the left | ||
| Some(non_backslash_index) => (index - non_backslash_index) % 2, | ||
| // Only `\` to the left | ||
| None => (index + 1) % 2, | ||
| }; | ||
| // Make sure break is after even number (including zero) of `\` |
There was a problem hiding this comment.
I wonder if you could elaborate more on these comments or rephrase them somehow. When I read them I feel like there are some details that are missing. For example, why do we need to make sure the break is after an even number?
There was a problem hiding this comment.
Enhanced the comments and moved some to a more appropriate place. I hope they are sufficiently clear now.
There was a problem hiding this comment.
Apologies for the delay on the re-review. After taking another look at this with the updated comments I feel like it's a lot clearer to understand what's going on with the code, and I think the logic that you've laid out to prevent breaking on on \\ characters makes sense.
I'm happy to move forward with this. We're waiting for a few PRs to land in rust-lang/rust, so going to mark this as ready-to-merge, but won't merge it just yet.
|
Are there any plans to merge this? I've just stumbled upon this bug on nightly. |
|
@purplesyringa thanks for reaching out, and sorry to hear that you hit this issue The team is getting ready for another release right now, and we'll likely revisit this PR for the following release. |
Backport #4524
Fixes #5138
Backport of #4524 that fixes #4471 with enhancements to fix #5138.