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

Custom highlight range color support #1900

Closed
wants to merge 1 commit into from

Conversation

bojan88
Copy link
Contributor

@bojan88 bojan88 commented Oct 11, 2021

Implements #1699, and related to #339

Basically, adds support for css hex color in line range - i.e. 30:40#ff0.

Please take this PR with a grain of salt. Note that it might not be going in the right direction that maintainers would like. I started working on #1699, and the change grew substantially, but then I saw that #339 is the preferred way to solve the issue.

This is just my idea how we could implement this, and maybe a place for further discussion. Please feel free to request changes or even completely discard this PR if you think this is not the way bat should evolve.

@bojan88 bojan88 force-pushed the custom-range-color branch from 375bddf to 18cb4de Compare October 11, 2021 22:43
@bojan88 bojan88 force-pushed the custom-range-color branch from 18cb4de to 590e361 Compare October 11, 2021 23:14
@@ -197,7 +197,7 @@ impl<'b> Controller<'b> {
printer: &mut dyn Printer,
writer: &mut dyn Write,
reader: &mut InputReader,
line_ranges: &LineRanges,
line_ranges: &LineRanges<crate::line_range::LineRange>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a strange error from cargo check --locked --target=x86_64-unknown-linux-gnu --verbose --lib --no-default-features --features regex-onig that LineRange was not in scope even thou there's use crate::line_range::LineRange; at the start of the file

@sharkdp
Copy link
Owner

sharkdp commented Oct 24, 2021

Interesting idea. Thank you for your contribution. The proposed format looks a bit strange at first, but it really adds a lot of flexibility. And I think it would be compatible with the format in #1841 as well?

What do the other maintainers think? @eth-p @keith-hall @Enselic?

@Enselic
Copy link
Collaborator

Enselic commented Oct 24, 2021

Thanks for taking the time to make a contribution @bojan88.

Like you realized yourself, doing something more generic in line with #339 would be better, I think.

I'm not sure it is a good idea to go ahead further with this PR, TBH. Doing so would make #339 harder to implement (because then you have to take compatibility with this into account), and #339 is still something that would be great to have.

@bojan88
Copy link
Contributor Author

bojan88 commented Oct 26, 2021

Sounds good. Feel free to close this anytime.
Or let me know if you want me to update it in case you think something similar might work.

@Enselic
Copy link
Collaborator

Enselic commented Oct 28, 2021

Thank you for your understanding. Let's close this for now then.

@Enselic Enselic closed this Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants