Refactor `write.rs` #119

ISSOtm · 2024-07-05T21:20:46Z

I'm aware this is a big PR, but I tried addressing that below the separation line.

The change that motivated this rewrite was allowing help and note messages to show up even on label-less diagnostics. (This turned out to be very difficult to implement in the current architecture, so I figured it wouldn't hurt to clean it up as a way to learn its ins and outs.)

The other main upshot is that the printing logic should be far less opaque and hairy, and thus more maintainable. To the best of my knowledge, there is only one breaking change here, the new Location struct. It can be removed if compat breakage is to be avoided.

As for this PR's scope, I seem to understand from looking more broadly at this repo's issue tracker, that you'd like to spend less time on this crate; perhaps more on ariadne2, perhaps on something else (including non-programming).

I understand and relate to the sentiment, and I respect your decision whatever its rationale.
I would also like to improve ariadne further, as it's a great crate that I'd like to see some kinks of ironed out for my own projects.
I think I can do my part in bringing these two together by volunteering to help with maintenance. I am willing to start owning the same code I started hacking on and tweaking, and going through the issues and PRs.

Let me know if you're interested—I'm also available at my commit email address, if you'd rather discuss that topic privately.
I'll also understand if you decline that offer and/or reject this PR, of course 😛

zesterer · 2024-07-07T14:03:59Z

Wow, this is a great PR! I'm going to try to get some time to look through this properly, but unfortunately I'm travelling for 2 weeks as of tomorrow so it'll have to be after that. I really appreciate the time spent on this though!

ISSOtm · 2024-07-07T21:20:52Z

No worries, then :)

As for the reviewing process, I tried making the commit log be easier to process piece-meal. (GitHub's UI allows you to review each commit's changes individually.) I think that should be all the more useful as the PR's cumulative changes are really large and complex.

Do let me know if you have doubts or questions about any part of this ^^

hellow554 · 2024-07-09T12:35:07Z

hey @ISSOtm

what an awesome PR.

One thing that bugged me a lot in ariadne is trailing whitespace in the output.
Your PR seemed to made it "worse" (not as an offend, please bear with me!)

What do you think: Would it be possible to remove trailing whitespaces in your code, or is that not possible in the current situation?

ISSOtm · 2024-07-09T14:32:52Z

FWIW, this is still as much whitespace as the current code, there's just more labels so it's more whitespace.

I can look into removing trailing whitespace, but does it cause any practical trouble?

hellow554 · 2024-07-10T05:50:30Z

Åh okay, it looked worse, sorry 🙈
Yes and no: I'm running ui tests on my program and often there's trailing white space that vscode likes to remove on every occasion.
Therefore I hoped that it can be "fixed"

ISSOtm · 2024-07-10T07:18:41Z

Sublime Text has a "only trim trailing whitespace on lines I modified myself" option, which is more appropriate for this. I don't know if VS Code has something similar.

ISSOtm · 2024-11-28T21:51:35Z

White rebasing this PR, I'm realising that there are several commits that could be cherry-picked into separate PRs. @zesterer, would you prefer if I did that?

ISSOtm · 2024-11-28T22:16:09Z

I've rebased this PR against main, and ensured that every single commit passes cargo test on its own, so commits can be cherry-picked as necessary.

fmt and clippy fail, but they also do on main. Let me know if you'd like PRs that fix those.

And also use a simpler, `log10`-based logic

This makes its purpose clearer, and also removes one pair of parens

Makes sense to have that logic next to the `Display` impl, IMO

This ensures only a single copy of the error message exists, and outlines yet more code outside of the massive `write_for_stream` function.

This improves dead code analysis

Just deflating the line count a bit

Missed it, because that one didn't have the `<unknown>` fallback. Introducing it should improve consistency a bit.

Further deflating `write_for_stream`, though it also should improve performance slightly: - The number of digits is only computed for the greatest line number - This is a valid transformation, because the largest line no is also one of the largest

Making its purpose clearer.

In the hopes of making the function a little shorter

This is more compact, and it might be cheaper too!

The field names are more expressive than a tuple

To determine what goes to the left of a "source group" designation

It makes the purpose of the pointer comparison clearer and is a little bit more concise.

Factor out most of the repeated code

Folding together mostly common cases

Use `collect()` for hopefully better Vec prealloc, derive one of the arrays from the other (since it's a subset of the first), and sort the first array to guarantee that the second one already is.

`format_args!` is more difficult to use (hence why `code` stays), but avoids allocating a `String`

It has more character variety, and thus is more likely to show differences that wouldn't be visible in ASCII

There aren't any labels left by that point

This eliminates a few parameters, and makes each "section"'s purpose more evident

zesterer · 2024-12-10T19:35:10Z

Thanks! Going to try to review this over the weekend.

This leads to slightly simpler logic

Then, we can avoid holding onto the Vec's len across the whole loop

For consistency with other crates, and `Note` and `Help`.

ISSOtm · 2024-12-12T10:00:54Z

Those checks don't fail locally, but this seems to be caused by a Rust version difference. I'm not sure which version I should be using, since the crate doesn't specify a MSRV.

ISSOtm force-pushed the write_refactor branch from b1eba07 to b77d74c Compare July 5, 2024 21:24

ISSOtm force-pushed the write_refactor branch from b77d74c to a9406d9 Compare November 28, 2024 22:09

ISSOtm added 20 commits December 10, 2024 20:03

Ignore temp files generated by cargo insta

f73e10a

Factor out a nb_digits function

ba7b721

And also use a simpler, `log10`-based logic

Dedicate a struct name to repeating chars

59790f3

This makes its purpose clearer, and also removes one pair of parens

Make obtaining its colour a method of ReportKind

da3e437

Makes sense to have that logic next to the `Display` impl, IMO

Factor out a fecth_source function

e215cc6

This ensures only a single copy of the error message exists, and outlines yet more code outside of the massive `write_for_stream` function.

Use bool::then_some to simplify a few exprs

8ba9c61

Remove dead impl Display for Show

9ea9f77

Mark Show and Rept as crate-local

07ceb6b

This improves dead code analysis

Simplify computation of line numbers a bit

465e3d6

Just deflating the line count a bit

Use fetch_source in Report::get_source_groups as well

4d33284

Missed it, because that one didn't have the `<unknown>` fallback. Introducing it should improve consistency a bit.

Factor out a max_line_no function

7c37877

Further deflating `write_for_stream`, though it also should improve performance slightly: - The number of digits is only computed for the greatest line number - This is a valid transformation, because the largest line no is also one of the largest

Force Show to only contain an Option

9b3a043

Making its purpose clearer.

Use more compact print!/write! syntax

d199caa

In the hopes of making the function a little shorter

Use the format! padding feature to pad out line numbers

c98d4f6

This is more compact, and it might be cheaper too!

Factor out a function to obtain the appropriate vbar

67215dc

Use a slightly more descriptive name for is_line

a2f5b69

[BREAKING] Factor out a Location struct to group that info

638c80c

The field names are more expressive than a tuple

Factor out a separate function for group_connector

992c9f3

To determine what goes to the left of a "source group" designation

Factor out a is_referencing method

a50004c

It makes the purpose of the pointer comparison clearer and is a little bit more concise.

Condense the arrow-character-selection logic

12067f0

Factor out most of the repeated code

ISSOtm added 11 commits December 10, 2024 20:03

Condense the arrow-char-selection margin logic a bit more

e8d3706

Folding together mostly common cases

Tweak collection of multi-line labels

caada34

Use `collect()` for hopefully better Vec prealloc, derive one of the arrays from the other (since it's a subset of the first), and sort the first array to guarantee that the second one already is.

Elide one alloc per report emitted

f4f5fa2

`format_args!` is more difficult to use (hence why `code` stays), but avoids allocating a `String`

Factor out a function for writing margin characters

6ca20a6

Switch snapshot tests to Unicode

db16ba4

It has more character variety, and thus is more likely to show differences that wouldn't be visible in ASCII

Add a few more tests

bf18d02

Build the line_labels array in a single statement

1f8e9a3

Factor out a function for checking if a label is the margin one

531a5e0

Avoid trying to render labels when printing help or notes

138006f

There aren't any labels left by that point

Split off write_margin into two functions

53c0c8a

This eliminates a few parameters, and makes each "section"'s purpose more evident

Remove an unused enumerate()

a14e462

ISSOtm force-pushed the write_refactor branch 3 times, most recently from c08f8e8 to dab841a Compare December 10, 2024 19:11

ISSOtm added 7 commits December 11, 2024 12:37

Add multi-source tests

059429f

Factor out a function to write spacer lines

493d7b5

Print spacers before groups, rather than after

fa4b46a

This leads to slightly simpler logic

Iterate on groups by ref instead of by value

2ea74f4

Then, we can avoid holding onto the Vec's len across the whole loop

Rearrange a bool expr like Clippy suggests

825d66e

Add a test for help and note but no labels

05524fd

Do not apply the report kind's colour to its colon

34fe426

For consistency with other crates, and `Note` and `Help`.

ISSOtm force-pushed the write_refactor branch from dab841a to 34fe426 Compare December 11, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `write.rs` #119

Refactor `write.rs` #119

ISSOtm commented Jul 5, 2024

zesterer commented Jul 7, 2024

ISSOtm commented Jul 7, 2024

hellow554 commented Jul 9, 2024

ISSOtm commented Jul 9, 2024

hellow554 commented Jul 10, 2024

ISSOtm commented Jul 10, 2024

ISSOtm commented Nov 28, 2024

ISSOtm commented Nov 28, 2024

zesterer commented Dec 10, 2024

ISSOtm commented Dec 12, 2024

Refactor write.rs #119

Are you sure you want to change the base?

Refactor write.rs #119

Conversation

ISSOtm commented Jul 5, 2024

zesterer commented Jul 7, 2024

ISSOtm commented Jul 7, 2024

hellow554 commented Jul 9, 2024

ISSOtm commented Jul 9, 2024

hellow554 commented Jul 10, 2024

ISSOtm commented Jul 10, 2024

ISSOtm commented Nov 28, 2024

ISSOtm commented Nov 28, 2024

zesterer commented Dec 10, 2024

ISSOtm commented Dec 12, 2024

Refactor `write.rs` #119

Refactor `write.rs` #119