cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Move to a global Renderer

Open Muscraft opened this issue 4 months ago • 1 comments

As I have been working on cargo's diagnostic system, I have disliked copying around the code for a new Renderer:

let renderer = Renderer::styled().term_width(
    gctx.shell()
        .err_width()
        .diagnostic_terminal_width()
        .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
);

It felt like a case where we should have one Renderer created in GlobalContext, and everything can reference that, so I did that in this PR.

Overall, I am happy with the amount of duplicate code this change removes. This change should help mitigate the risk of updating duplicate code and missing one. It should also make it harder to get into scenarios where gctx.shell() is borrowed twice, leading to panics. This problem was one I ran into early on as I tried to write messages to stderr like so:

writeln!(
    gctx.shell().err(),
    "{}",
    Renderer::styled()
        .term_width(
            gctx.shell()
                .err_width()
                .diagnostic_terminal_width()
                .unwrap_or(annotate_snippets::renderer::DEFAULT_TERM_WIDTH),
        )
        .render(message)
)?;

Note: This PR includes updating annotate-snippets to 0.11.2, as it added #[derive(Debug)] to Renderer, which was needed for Renderer to be added to GlobalContext

Muscraft avatar Apr 27 '24 18:04 Muscraft

r? @epage

rustbot has assigned @epage. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot avatar Apr 27 '24 18:04 rustbot

I'm a bit uncomfortable generating this in Context::new because it can be quite easy for us to add config for controlling this (honestly, I'm surprised it doesn't exist).

As for the terminal width changing, that is more likely to come up when it comes to cargo watch support (#9339).

So far, a Renderer is "cheap". We could instead make the renderer on every call.

epage avatar Apr 29 '24 15:04 epage

@bors r+

Thank you all :)

weihanglo avatar Apr 30 '24 18:04 weihanglo

:pushpin: Commit 5415b04fddc0039ef4111a4908b89bf2c2cb764c has been approved by weihanglo

It is now in the queue for this repository.

bors avatar Apr 30 '24 18:04 bors

:hourglass: Testing commit 5415b04fddc0039ef4111a4908b89bf2c2cb764c with merge a1f8e450b373c111c10330bd33c833928ef575f8...

bors avatar Apr 30 '24 18:04 bors

:sunny: Test successful - checks-actions Approved by: weihanglo Pushing a1f8e450b373c111c10330bd33c833928ef575f8 to master...

bors avatar Apr 30 '24 18:04 bors