ariadne
ariadne copied to clipboard
zesterer
Refactor `write.rs`
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:
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!
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 ^^
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?
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?
Å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"
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.
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?
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.
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.