Refactor write.rs
#119
Refactor write.rs
#119
Conversation
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.
They tend to create untracked files on test failures, and that's a tiny bit annoying
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
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`.
|
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.
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 🙈 |
|
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. |
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
helpandnotemessages 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
Locationstruct. 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 😛