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

refactor: add convenience HighlightLines::from_state constructor #569

Conversation

alexpasmantier
Copy link
Contributor

This PR adds a convenience method to easily be able to reinstantiate a HighlightLines object from a previous state the user might have cached.

@alexpasmantier alexpasmantier force-pushed the refactor/convenience-highlight-from-previous-state branch from dba6d94 to bfbc619 Compare January 18, 2025 19:08
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Personally I would prefer a test that demonstrates how to use this new function, that also serves as a regression test.

But I'm fine you merging this as-is Keith if you are confident this is needed and the right approach.

@keith-hall
Copy link
Collaborator

I'm confident that this is needed and the right approach. I also agree that a test would be nice 🙂

@alexpasmantier
Copy link
Contributor Author

Thank you both for your input 🙏🏻

I added a syntect::easy::HighlightLines<'a>::state method, without which the first one wouldn't be very useful and an associated test.

Didn't add the test as an example to the method's documentation but can do so if you think it's a good idea.

Cheers

Copy link
Collaborator

@keith-hall keith-hall left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the test. Looks good to me

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks good to me too, thanks.

I'd like to ask trishume to allow CI to run without manual action, but that's a later thing.

@alexpasmantier
Copy link
Contributor Author

Thanks for the quick feedback 🙏
So should we merge this?

@keith-hall keith-hall merged commit 7fe13c0 into trishume:master Jan 24, 2025
4 checks passed
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