feat: Add full label printing support#152
Conversation
|
Thanks for the contribution! Would it be possible for you to expand a little on the motivation for this? I'd think that showing the full contents of a file would often not be a positive thing. |
|
The change doesn't show the full file, just the full span of the label. The motivation was that I wasn't happy with not being able to see the full context of short labels even when there wouldn't be any problem displaying the whole thing. I guess a better API here could be a maximum line count before ellipses kick in instead of just a boolean ellipses always/full label always. If you want I could give implementing that a show instead of having this PR merged. |
zesterer
left a comment
zesterer
left a comment
There was a problem hiding this comment.
Apologies, that makes more sense. Just two comments, but otherwise I'm happy to see this merged!
src/write.rs
Outdated
|
|
||
| #[test] | ||
| fn multiline_label_show_full() { | ||
| let source = "apple\n==\norange"; |
There was a problem hiding this comment.
Would it be possible to change the string to "apple\n==\norange\npear"? That ensures that we're also not accidentally displaying the full file, only the lines covered by the span.
src/lib.rs
Outdated
| /// Should this report not collapse multiline labels? | ||
| /// | ||
| /// If unspecified, this defaults to 'false' | ||
| pub const fn with_show_full(mut self, show_full: bool) -> Self { |
There was a problem hiding this comment.
My feeling is that this function name / comment don't quite describe how this works. I'd suggest with_show_full_span and
/// Whether all lines covered by a multi-line span are shown, instead of just the first and last lines.
|
Made the tweaks and also implemented a maximum line count option for the config. |
|
Thanks! Looks like it needs a |
|
Oops. Fixed. |
|
In an unrelated change, I added the ability to specify the number of lines of context that appear around label ends. This seems to have produced some conflicts. I'm also not entirely convinced that |
zesterer
force-pushed
the
main
branch
2 times, most recently
from
2f36bc2 to
4b3807c
Compare
|
I moved the collapse settings from the config to the individual label. If this looks good to you, I can also make a PR to do the same to make the number of contexts lines label specific as well. |
|
Hello! Just to make you aware: Ariadne's development has moved to Codeberg. If you're interested in continuing this PR there, it seems like the migration tool recreated it: https://codeberg.org/zesterer/ariadne/pulls/152. |
2 participants
This adds a simple boolean to the Report builder to not collapse multiline diffs with ellipses.