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

Make parts of html API return Result with new syntect::Error #424

Merged
merged 1 commit into from Mar 1, 2022

Conversation

Enselic
Copy link
Collaborator

@Enselic Enselic commented Feb 20, 2022

Introduce a new error type syntect::Error that can represent any error
that can happen in syntect, and not only LoadingErrors.

In an upcoming PR, a syntect::parsing::ParsingError will be introduced
for highlight_line(). But to keep the PR small and easy to review, I wait with that.

Using https://github.com/Enselic/cargo-public-items one can easily see what changes to the public API this PR do. Excluding pure additions to the API; the changes are the following:

pub fn syntect::html::append_highlighted_html_for_styled_line(v: &[(Style,&str)], bg: IncludeBackground, s: &mut String)
becomes
pub fn syntect::html::append_highlighted_html_for_styled_line(v: &[(Style,&str)], bg: IncludeBackground, s: &mut String) -> Result<(), Error>
pub fn syntect::html::highlighted_html_for_file<P: AsRef<Path>>(path: P, ss: &SyntaxSet, theme: &Theme) -> io::Result<String>
becomes
pub fn syntect::html::highlighted_html_for_file<P: AsRef<Path>>(path: P, ss: &SyntaxSet, theme: &Theme) -> Result<String, Error>
pub fn syntect::html::highlighted_html_for_string(s: &str, ss: &SyntaxSet, syntax: &SyntaxReference, theme: &Theme) -> String
becomes
pub fn syntect::html::highlighted_html_for_string(s: &str, ss: &SyntaxSet, syntax: &SyntaxReference, theme: &Theme) -> Result<String, Error>
pub fn syntect::html::styled_line_to_highlighted_html(v: &[(Style,&str)], bg: IncludeBackground) -> String
becomes
pub fn syntect::html::styled_line_to_highlighted_html(v: &[(Style,&str)], bg: IncludeBackground) -> Result<String, Error>

The reason we want to change these is so that when highlight_line() starts to return a Result, an error can continue to propagate upwards through the above functions. The above functions makes use of highlight_line().

I don't think it is worth the hassle to rename the new functions so that we can have a deprecation message. I think it will be pretty obvious for clients what to do when they get compilation errors. They can simply add an .unwrap() to essentially get the same behavior as before. This will of course also be mentioned in the CHANGELOG.md in #409

This is one step towards a solution for #98

Introduce a new error type `syntect::Error` that can represent any error
that can happen in syntect, and not only `LoadingError`s.

In an upcoming PR, a `syntect::parsing::ParsingError` will be introduced
for `highlight_line`.
@trishume trishume merged commit 8e1dac8 into trishume:master Mar 1, 2022
@trishume
Copy link
Owner

trishume commented Mar 1, 2022

By the way, given how bad I am at reviewing, you can feel free to use the trick where you submit multiple successive PRs at once and just change their base branches to the previous PR. That way when I do get around to reviewing I can review an entire chain as opposed to taking N cycles to get your changes in.

@Enselic Enselic deleted the syntect-error branch March 1, 2022 06:13
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.

None yet

2 participants