Fix line number being reset on each Indented write_str call#18
Fix line number being reset on each Indented write_str call#18ten3roberts merged 1 commit intoeyre-rs:masterfrom ComunidadAylas:master
Conversation
|
Hey, is there any chance of this and other PRs being reviewed? I'd love to get the ball rolling somehow, but I don't know if e.g. removing obsolete Clippy lint warnings is in the scope of this PR, or even a concern against merging it. |
ten3roberts
left a comment
ten3roberts
left a comment
There was a problem hiding this comment.
Looks good, and will likely save us many headaches down the line in formatters getting the wrong output.
Maintenance has returned to eyre and we have assembled a small team to deal with the busfactor, as well as making plans for moving all of eyre forwards, so contributions are very welcome at the momen 😊
Removing the obsolete clippy warnings are not necassarily, as those will be fixed when we move indenter to a monorepo underneath eyre/eyre.
|
Sweet, thanks for the heads-up and nice to hear that! ❤️ |
|
Sincere apologies for the ping, I typically avoid pinging like this, especially given the stale-ness of the repo and one not having previously committed to the repo, but... I just spent the last few hours trying to figure out why my app's serde errors were getting some unpleasant color bleed. 
I went so far as to make a custom Writer in Eyre that breaks Error's Display outputs into individually-styled lines, to make sure each "0:" has it's style properly reset. ..Except that didn't even even work! It always had the color bleed. And to make things even stranger, when I changed a you can imagine my surprise when the output looked entirely different, and in fact better: 
And so I dug deeper into the issues and pull requests in the eyre-rs organization, and came across someone fixing this exact bug years ago. I understand this repository hasn't had much activity in a while, but this is a simple, non-conflicting, already-approved change that I think many would appreciate. :)
No longer needed! Just use |
|
Grah Tests work locally, but can't merge as CI seems to hang, and don't have perms to see what is going on with it or force merge 😢 |
I'm so glad to be the one who fixed that exact bug years ago, thank you! I guess I’m living up to the tagline in my profile: doing stuff people didn't realize they wanted... 😂 I'll try rebasing this branch to see if that helps with CI. If not, it's not the end of the world either, as applications that don't need to publish to crates.io can just patch the dependency, as @nullstalgia mentioned 🙂 |
Head branch was pushed to by a user without write access
|
Seems the CI is simply borked, not surprised given the workflow file hasn't been updated in >5 years. If @yaahc isn't available to force-merge (or possibly still unaware of this PR?), it might be possible to open another PR to update the CI workflow and then merge this after that one, or for a contributor with write access to modify this one? I honestly forget if PRs use the repository's existing workflow or the one they're submitting within their changes. In any case, I have submitted a PR with a fixed CI workflow ( #22 ), hopefully this can help grease the wheels. |
They use the workflows they're submitting with their changes, but if the repository has any required checks set up before merging, the PR could still get blocked from merging if the PR introduces an incompatible set of workflows. And not every person with write access to the repository can tweak the required checks. I'll rebase this again and see if your PR helped! This is only my second oldest open PR, after all 🙏 😂 |
In most realistic scenarios, the `write_str` method provided by
core::fmt::Write implementations is called several times, as this is
what allows to implement allocation-free string interpolation. When this
happens, each string passed to `write_str` can only be assumed to be a
slice of the text to be written.
However, the `write_str` implementation provided by the `Indented`
struct does not keep track of the number of lines written so far,
relying on the input string slice to represent the line count of the
whole text. As a consequence, when several strings are written out,
formatters will be provided with incorrect line count information:
```
indented.write_str("hello")?; // ind = 0 => Format line_no = 0
indented.write_char('\n')?;
indented.write_str("world")?; // ind = 0, but this is not the first line!
```
To improve on this, let's make Indented remember the number of lines
that have been written so far, and pass that to the formatters.
While at it, I've added the `empty_lines` test as a sanity check for
proper empty line handling, which should be the same as before. I've
also added the `several_interpolations_keep_monotonic_line_numbers`
test to check that these changes work as expected. Previously, that test
failed with this output:
```
thread 'tests::several_interpolations_keep_monotonic_line_numbers' panicked at 'assertion failed: `(left == right)`
left: `"verify\n this\n and this"`,
right: `"verify\nthis\nand this"`'
```
|
I can't believe this PR finally got merged! 🎉 To my surprise, the past couple of days turned out to be some of the most fun I've had working on a PR due to a kind of bizarre comedy that I don't know how to explain. Huge thanks to @ten3roberts and @nullstalgia! 😄 |
|
A pleasure I can see what I can do in regards to getting this patch published |
|
should be released now, ty for the patience 😅 |
4 participants
In most realistic scenarios, the
write_strmethod provided bycore::fmt::Writeimplementations is called several times, as this is what allows to implement allocation-free string interpolation. When this happens, each string passed towrite_strcan only be assumed to be a slice of the text to be written.However, the
write_strimplementation provided by theIndentedstruct does not keep track of the number of lines written so far, relying on the input string slice to represent the line count of the whole text. As a consequence, when several strings are written out, formatters will be provided with incorrect line count information:To improve on this, let's make Indented remember the number of lines that have been written so far, and pass that to the formatters.
While at it, I've added the
empty_linestest as a sanity check for proper empty line handling, which should be the same as before. I've also added theseveral_interpolations_keep_monotonic_line_numberstest to check that these changes work as expected. Previously, that test failed with this output: