bat icon indicating copy to clipboard operation
bat copied to clipboard

Add `bat::PrettyPrinter::clear_highlights`

Open rhysd opened this issue 4 years ago • 3 comments

Fixes #1919

With this API, the code written in #1919 can be improved as follows:

use bat::PrettyPrinter;

let targets: (PathBuf, Vec<(usize, usize)>) = vec![];

let mut pp = PrettyPrinter::new();
for (path, ranges) in targets.iter() {
    pp.input_file(path);
    for (start, end) in ranges.iter() {
        pp.highlight_range(*start, *end);
    }

    pp.print().unwrap();
    // Clear highlights for next iteration
    pp.clear_highlights();
}

rhysd avatar Oct 23 '21 12:10 rhysd

This request/bug-ticket looks very reasonable to me - thank you! However, do we really need a new public method for that? Shouldn't clear_highlights be called automatically after .print()ing?

sharkdp avatar Oct 24 '21 10:10 sharkdp

@sharkdp

Shouldn't clear_highlights be called automatically after .print()ing?

It also makes sense.

https://github.com/sharkdp/bat/blob/7fe4fdf33df3aa958b3263a939a8b24b9d06f534/src/pretty_printer.rs#L242-L248

The line ranges are cloned into config.highlighted_lines at L248. So the line ranges in PrettyPrinter instance does not change currently.

Since inputs are cleared, clearing line ranges makes sense to me. The point is that it is a breaking change in API. I avoided the breaking change but clearing the line ranges at print() is sufficient for me. Do you think the breaking change is OK?

let hls = std::mem::take(&mut self.highlighted_lines);
self.config.highlighted_lines = HighlightedLineRanges(LineRanges::from(hls));

rhysd avatar Oct 26 '21 07:10 rhysd

The point is that it is a breaking change in API. I avoided the breaking change but clearing the line ranges at print() is sufficient for me. Do you think the breaking change is OK?

Not sure I understand this correctly: wouldn't the breaking change only affect the cases where the behavior is currently broken/buggy anyway? (#1919)

sharkdp avatar Nov 22 '21 19:11 sharkdp

@rhysd Can you provide a quick update please? Are you still interested in this change? Or has the situation improved due to syntax lazy loading (see accompanying ticket)?

sharkdp avatar Sep 02 '22 21:09 sharkdp

I'm sorry for the lack of response. I forgot this PR. I'll take a look tonight.

rhysd avatar Sep 05 '22 08:09 rhysd

wouldn't the breaking change only affect the cases where the behavior is currently broken/buggy anyway?

I'm not sure it is broken/buggy, but I agree that it is very edge case.

rhysd avatar Sep 05 '22 12:09 rhysd

@sharkdp I updated this branch at 3d7817d. Would you review changes?

rhysd avatar Sep 05 '22 12:09 rhysd

Looks great - thank you. I like the implementation :+1:

sharkdp avatar Sep 06 '22 07:09 sharkdp