zesterer/ariadne
1
33
Fork
You've already forked ariadne
4

feat: Add full label printing support #152

Open
TurtleArmyMc wants to merge 1 commit from TurtleArmyMc/main into main
pull from: TurtleArmyMc/main
merge into: zesterer:main
zesterer:main
zesterer:optional-spans
zesterer:keep-compact-gutter
zesterer:location-only-layout
zesterer:woodpecker-ci
zesterer:suggestions
zesterer:ariadne2
zesterer:api-redesign
TurtleArmyMc commented 2025年10月11日 01:21:40 +02:00 (Migrated from github.com)
Copy link

This adds a simple boolean to the Report builder to not collapse multiline diffs with ellipses.

This adds a simple boolean to the Report builder to not collapse multiline diffs with ellipses.

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.

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.
TurtleArmyMc commented 2025年10月29日 04:00:38 +01:00 (Migrated from github.com)
Copy link

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.

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
Copy link

Apologies, that makes more sense. Just two comments, but otherwise I'm happy to see this merged!

Apologies, that makes more sense. Just two comments, but otherwise I'm happy to see this merged!
Owner
Copy link

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.
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. ```
Owner
Copy link

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.

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.
TurtleArmyMc commented 2025年10月31日 03:47:29 +01:00 (Migrated from github.com)
Copy link

Made the tweaks and also implemented a maximum line count option for the config.

Made the tweaks and also implemented a maximum line count option for the config.

Thanks! Looks like it needs a cargo fmt before merge, but I'm happy with this now.

Thanks! Looks like it needs a `cargo fmt` before merge, but I'm happy with this now.
TurtleArmyMc commented 2025年11月03日 23:12:30 +01:00 (Migrated from github.com)
Copy link

Oops. Fixed.

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 Config is the place to put this option, and it might be better suited to being attached to individual labels (since some are more important than others). What are your thoughts on this?

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 `Config` is the place to put this option, and it might be better suited to being attached to individual labels (since some are more important than others). What are your thoughts on this?
TurtleArmyMc commented 2025年12月20日 01:19:11 +01:00 (Migrated from github.com)
Copy link

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.

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.
This pull request has changes conflicting with the target branch.
  • src/lib.rs
  • src/write.rs
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin TurtleArmyMc/main:TurtleArmyMc/main
git switch TurtleArmyMc/main
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
zesterer/ariadne!152
Reference in a new issue
zesterer/ariadne
No description provided.
Delete branch "TurtleArmyMc/main"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?