search

zesterer / ariadne

A fancy diagnostics & error reporting crate
https://crates.io/crates/ariadne
MIT License
1.69k stars 74 forks source link

Refactor `write.rs` #119

Open ISSOtm opened 3 weeks ago

ISSOtm commented 3 weeks ago

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 :stuck_out_tongue:

zesterer commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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!)

grafik

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 commented 2 weeks ago

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 commented 2 weeks ago

Å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 commented 2 weeks ago

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.