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

Add bat::PrettyPrinter::clear_highlights #1920

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Oct 23, 2021

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
Copy link
Owner

sharkdp commented Oct 24, 2021

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
Copy link
Contributor Author

rhysd commented Oct 26, 2021

@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
Copy link
Owner

sharkdp commented Nov 22, 2021

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
Copy link
Owner

sharkdp commented Sep 2, 2022

@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
Copy link
Contributor Author

rhysd commented Sep 5, 2022

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

@rhysd
Copy link
Contributor Author

rhysd commented Sep 5, 2022

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
Copy link
Contributor Author

rhysd commented Sep 5, 2022

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

@sharkdp
Copy link
Owner

sharkdp commented Sep 6, 2022

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