std::fmt traits compatible version of PrettyPrinter::print

[yaahc]

As far as I can tell bat as a library only supports writing to stdout, I'm interested in seeing if I can integrate bat with color-backtrace/color-spantrace and eyre. To do this I need bat to write to a std::fmt::Formatter and return a fmt::Result instead of printing to stdout (i assume?) and returning a bat::error::Error.

So assuming I'm not misreading the docs...

Proposal

Implement the Display trait for PrettyPrinter where <PrettyPrinter as fmt::Display>::fmt acts as an alternative to PrettyPrinter::print.

[sharkdp]

Sounds good to me 👍

eth-p added the "good first issue" label

are you sure? 😄 I'm afraid that might be a bit more involved (especially the error handling part), but I could be wrong.

[eth-p]

It might not be the easiest first issue out there, but it's a great way to learn Rust "by fire", so to speak 😂. The way everything comes together in bat is also pretty simple to understand, and it shouldn't take too long for someone to figure out how it works.

If you think we should remove the label because of how involved the process of integrating it would be, I wouldn't be against it though haha.

[yaahc]

are you sure?

I should note that I had to make similar changes to color-backtrace and there are ... problems ... that need to be resolved.

Specifically, I don't know what kind of windows support y'all have, but it could not be fixed in a backwards compatible way for color-backtrace because old windows color support uses sys calls between prints which is not easily (at all?) compatible with std::fmt.

athre0z/color-backtrace#27 for reference

A quick look at your toml only shows ansi color code deps so this may be a non issue.

[eth-p]

A quick look at your toml only shows ansi color code deps so this may be a non issue.

To do color we enable ANSI support in the Windows terminal, so there shouldn't be any issues as far as side-effects go. The error handling might be a bit more complex, but capturing the output as a string (to write to the formatter) or supporting writing to arbitrary &mut dyn Writes didn't seem like it would require too much more than creating a new OutputType for it :)

[niklasmohrin]

Hey there,

I was trying to implement writing to a &mut dyn Write in Controller and that worked okay, but I then realized, that the print function of PrettyPrinter takes &mut self (to move the inputs vector out of self and write to self.config), but std::fmt::Display is defined as fn fmt(&self, f: &mut Formatter) -> Result<(), Error>. Can someone tell me what to do now? I know there is RefCell, but I don't know if it is right for this situation (especially because the overhead of RefCell would apply to all methods).

supporting writing to arbitrary &mut dyn Writes didn't seem like it would require too much more than creating a new OutputType for it

To me, it looks like OutputType is only used inside functions of Controller but never as an argument. Couldn't there just be a (new) function that takes a &mut dyn Write?

---

Thanks for your work on bat everyone :)

[hedonhermdev]

Is this issue open? Can I work on it?

[niklasmohrin]

@hedonhermdev I for one am not working on it

[sharkdp]

@yaahc If you are still interested in this, maybe you could answer the questions by @niklasmohrin? Otherwise, I'd like to close this.

[yaahc]

@yaahc If you are still interested in this, maybe you could answer the questions by @niklasmohrin? Otherwise, I'd like to close this.

I'm not sure the exact best way to make this work within bat but this is similar to a problem I run into a lot in color-eyre. For example, we have a PanicHook which stores a config for how it will print PanicInfos which is probably analogous to PrettyPrinter. When it prints PanicInfos instead of storing them internally it has this panic_report method which takes the input and the PanicHook and returns a new type which wraps both and implements Display.

• https://docs.rs/color-eyre/0.5.10/color_eyre/config/struct.PanicHook.html#method.panic_report
• https://docs.rs/color-eyre/0.5.10/color_eyre/config/struct.PanicReport.html

You might be able to add an alternative to print that just removes the Inputs from self and then returns them in a new type which implements Display.

[botahamec]

I took a look at this, and found a problem which leads to this being way harder than expected. Here's what I have written:

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
    for (index, input) in self.inputs.into_iter().enumerate() {
        let is_first = index == 0;
        let result = if input.is_stdin() {
           self.controller
               .print_input(input, f, io::stdin().lock(), None, is_first)
        } else {
            // Use dummy stdin since stdin is actually not used (#1902)
            self.controller
                .print_input(input, f, io::empty(), None, is_first)
        };
        if let Err(error) = result {
            return Err(fmt::Error);
        }
    }

    Ok(())
}

The call to print_input actually results in a compiler error, because f doesn't implement std::io::Write. It implements std::fmt::Write

error[E0277]: the trait bound `Formatter<'_>: std::io::Write` is not satisfied
   --> src/controller.rs:282:41
    |
282 |                     .print_input(input, &mut f, io::empty(), None, is_first)
    |                                         ^^^^^^ the trait `std::io::Write` is not implemented for `Formatter<'_>`
    |
    = note: required because of the requirements on the impl of `std::io::Write` for `&mut Formatter<'_>`
    = note: required for the cast to the object type `dyn std::io::Write`

The Printer trait requires io::Write, which is unfortunate, considering what it's used for. According to the documentation for std::fmt::Write:

This trait only accepts UTF-8–encoded data and is not flushable. If you only want to accept Unicode and you don’t need flushing, you should implement this trait; otherwise you should implement std::io::Write.

I think Printer should use fmt::Write, but that's a breaking change

[Enselic]

Thank you for working on this.

I think Printer should use fmt::Write, but that's a breaking change

If we need to do a breaking change that's fine, we want to do that before v1.0.0 anyway.

I'm removing good first issue now though (as discussed earlier) because this all seems a bit tricky to get right.

[pythops]

very much needed feature 🙏
can't wait for it

[Piturnah]

Stumbled upon this issue while trying to use Bat for syntax highlighting in Gex.

Forgive me if this is naive, but I think that perhaps rather than implementing fmt::Display, it could be an easier solution to instead have an OutputType to write to anything that implements fmt::Write. Then the library user can provide some buffer in the config, and the print implementation would (probably) be as easy as changing print!("{}", foo) to write!(&mut buf, "{}", foo).

If this sounds reasonable I'd be happy to give a go working on an implementation.