Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider the hard-coding of ... as an indicator that a line has been trimmed or cut #174

Open
BurntSushi opened this issue Jan 9, 2025 · 11 comments

Comments

@BurntSushi
Copy link
Member

BurntSushi commented Jan 9, 2025

Ref #172

The high level problem I ran into with integrating annotate-snippets into Rust---and specifically, an upgrade from 0.9 to 0.11---was that trimming long lines automatically inserts a .... This is somewhat problematic in Python specifically because a ... is itself valid Python and not that uncommon. For example, with ..., it's hard to tell the difference between a line being trimmed and an actual line. Here's an actual example where it's ambiguous:

use annotate_snippets::{Level, Renderer, Snippet};

fn main() {
    // Taken from: https://docs.python.org/3/library/typing.html#annotating-callable-objects
    let source = "def __call__(self, *vals: bytes, maxlen: int | None = None) -> list[bytes]: ...";
    let src_annotation = Level::Error.span(4..12).label("annotation");
    let snippet =
        Snippet::source(source).line_start(1).annotation(src_annotation);
    let message = Level::Error.title("").snippet(snippet);

    println!("{}", Renderer::plain().term_width(83).render(message));
}

I futzed with the terminal width here a bit to make it ambiguous for this specific snippet, but I think this gets the idea across.

In #172, I tried to approach this problem by making it possible to override the indicator to something else. For us, we're thinking of using (U+2026 HORIZONTAL ELLIPSIS) since it gets the same idea across, but should hopefully be less confusable with actual Python syntax. Of course, I think it's true that any choice here could always risk being confusable, but ... is far less likely to result in ambiguity for, e.g., Rust code than it is for Python code.

@BurntSushi
Copy link
Member Author

Popping up a level, is it useful if I keep sending patches to this project? Putting myself in y'all's shoes, I'd be tempted to constrain the complexity of this specific project and reject patches like this that arguably increase API complexity and potentially also the layout logic as well. I feel like coupling for a project like this is a great simplifier.

(I can still upsteam what I think are bug fixes as I come across them, so long as our code doesn't diverge too much internally.)

@epage
Copy link
Contributor

epage commented Jan 9, 2025

Popping up a level, is it useful if I keep sending patches to this project? Putting myself in y'all's shoes, I'd be tempted to constrain the complexity of this specific project and reject patches like this that arguably increase API complexity and potentially also the layout logic as well. I feel like coupling for a project like this is a great simplifier.

Like I said in another place, our primary focus is on cargo/rustc while we figure out what is needed for the current stage and then we'll be a bit more open to explore what generalizations make sense. Personally, I'd rather it not be completely themable because I suspect that will lead to too much complexity and having some level of consistency and sharing of best practices would be good, kind of like how I push back on some clap customization to encourage people making clap better for everyone or because consistency might be better than their slight optimization for their one use (besides binary size / compile times)

This doesn't mean we are against supporting outside needs but it won't get prioritized or it might get pushed back. I can understand not wanting to block your development when you have a product to deliver and working off of a fork.

@BurntSushi
Copy link
Member Author

All righty, that makes sense! In the interest of being concrete, here's what I'll do for now (unless they're are any objections or other opinions):

  • I'll file issues about use cases that might imply API additions.
  • When possible, I'll PR what I think are bug fixes.

Happy to revisit this as needed.

@epage
Copy link
Contributor

epage commented Jan 9, 2025

In #172, I tried to approach this problem by making it possible to override the indicator to something else. For us, we're thinking of using … (U+2026 HORIZONTAL ELLIPSIS) since it gets the same idea across, but should hopefully be less confusable with actual Python syntax. Of course, I think it's true that any choice here could always risk being confusable, but ... is far less likely to result in ambiguity for, e.g., Rust code than it is for Python code.

As I mentioned in #172, there are two potential problems with this

  1. If we can find a solution that works for both projects, then that seems even better than having to specialize this
  2. We are considering what unicode support would look like. We could allow injecting each rendering element (like I do with termtree, wow I need to document that) but that seems like that could be complex to configure and complex to implement. Instead we were thinking of a simple boolean to switch the type of rendering. Allowing this to be injected runs a bit counter to that and may cause problems with the boolean unicode plan.

I like the idea of using that unicode character but that would be blocked on having the rest of our unicode styling and a fallback would be needed for ASCII.

It also seems like the elided text should be styled differently than source. Maybe dimmed? Or to style the same as line_no?

@epage
Copy link
Contributor

epage commented Jan 9, 2025

I'd say feel free to open issues for anything you don't upstream, and note in it that your fork has it implemented, so that we can double check your assumptions of how well we can adopt it now and to make sure we are keeping in mind wider needs as we design the current stages.

Like I said, I would love there to be as little to no divergence as possible.

@epage
Copy link
Contributor

epage commented Jan 9, 2025

@BurntSushi it looks like we already color ... as line_no

if self.margin.was_cut_left() {
// We have stripped some code/whitespace from the beginning, make it clear.
buffer.puts(line_offset, code_offset, "...", *lineno_color);
}
if self.margin.was_cut_right(line_len) {
buffer.puts(line_offset, code_offset + taken - 3, "...", *lineno_color);
}

For an example screenshot, see https://github.com/rust-lang/annotate-snippets-rs/blob/master/tests/fixtures/color/strip_line_non_ws.svg

Are you customizing the colors, is the color no enough, or is the concern for color-stripped output?

@BurntSushi
Copy link
Member Author

I think that definitely helps, but I'm still a little concerned about relying on color for resolving ambiguity. Color might not be enabled for one reason or another (there are plenty of times where I myself will be looking at rustc diagnostics, for example, without color). And I guess there are accessibility concerns there too, although I'm not an expert in that domain. Like, I do think it's reasonable to use color as a mitigating factor here since, as I mentioned, I don't think it's possible to be perfect.

cc @MichaReiser if you have any opinions here as the person who originally brought up this concern. The context here I think is seeing whether we can come to a consensus on one string for indicating a cut line instead of making it configurable. (If I'm understanding @epage correctly here.)

@BurntSushi
Copy link
Member Author

Also, I will confess that I don't quite understand the intricacies of making the cut indicator a non-ASCII string. Basically, I think this

We are considering what unicode support would look like. We could allow injecting each rendering element (like I do with termtree, wow I need to document that) but that seems like that could be complex to configure and complex to implement. Instead we were thinking of a simple boolean to switch the type of rendering. Allowing this to be injected runs a bit counter to that and may cause problems with the boolean unicode plan.

and this

I like the idea of using that unicode character but that would be blocked on having the rest of our unicode styling and a fallback would be needed for ASCII.

went over my head. My knowledge of annotate-snippets is based on some light hacking around its code and really nothing else. I don't have a lot of context on its design and what has led to this point.

@MichaReiser
Copy link

MichaReiser commented Jan 10, 2025

Colors definitely help. I wasn't aware of the coloring because it doesn't show in our snapshots. It would still be great to have a non-color indicator that distinguishes the trimming sequence from a regular ... in Python for accessibility reasons (and when piping the output). But I must admit, I don't see an obvious choice for what other indicator to use.

@epage
Copy link
Contributor

epage commented Jan 10, 2025

Cargo is very conservative in what terminals it supports, so it unicode support is gated behind https://crates.io/crates/supports-unicode with a term.unicode config override, so we need separate ascii and unicode modes.

I'm unsure what @Muscraft has planned for unicode support. In theory, a bool could be thrown on the renderer and offer this but I also know Muscraft is in the middle of a big change to the renderer.

@epage
Copy link
Contributor

epage commented Jan 10, 2025

We snapshot with snapbox which can render escape codes as an svg. Cargo and clap also use that. Rustc integrated the underlying library, anstyle-svg, into their snapshotting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants