Skip to content

Add bat::PrettyPrinter::clear_highlights#1920

Merged
sharkdp merged 3 commits into
sharkdp:masterfrom
rhysd:issue-1919
Sep 6, 2022
Merged

Add bat::PrettyPrinter::clear_highlights#1920
sharkdp merged 3 commits into
sharkdp:masterfrom
rhysd:issue-1919

Conversation

@rhysd

@rhysd rhysd commented Oct 23, 2021

Copy link
Copy Markdown
Contributor

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();
}

@sharkdp

sharkdp commented Oct 24, 2021

Copy link
Copy Markdown
Owner

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?

@rhysd

rhysd commented Oct 26, 2021

Copy link
Copy Markdown
Contributor Author

@sharkdp

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

It also makes sense.

bat/src/pretty_printer.rs

Lines 242 to 248 in 7fe4fdf

/// Pretty-print all specified inputs. This method will "use" all stored inputs.
/// If you want to call 'print' multiple times, you have to call the appropriate
/// input_* methods again.
pub fn print(&mut self) -> Result<bool> {
self.config.highlighted_lines =
HighlightedLineRanges(LineRanges::from(self.highlighted_lines.clone()));

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));

@sharkdp

sharkdp commented Nov 22, 2021

Copy link
Copy Markdown
Owner

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 sharkdp added this to the v0.19 milestone Nov 22, 2021
@Enselic Enselic added the waiting-on-author Progress on this PR is blocked mostly because we are waiting on the author of the PR to do something label Feb 6, 2022
@keith-hall keith-hall removed this from the v0.19 milestone Feb 12, 2022
@sharkdp

sharkdp commented Sep 2, 2022

Copy link
Copy Markdown
Owner

@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)?

@rhysd

rhysd commented Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

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

@rhysd

rhysd commented Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

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

rhysd commented Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

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

@sharkdp

sharkdp commented Sep 6, 2022

Copy link
Copy Markdown
Owner

Looks great - thank you. I like the implementation 👍

@sharkdp sharkdp merged commit 0e03dce into sharkdp:master Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-author Progress on this PR is blocked mostly because we are waiting on the author of the PR to do something

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot clear highlights in bat::PrettyPrinter instance

5 participants