Skip to content

Implement Error and Display for all error enums by using thiserror#420

Merged
trishume merged 2 commits into
trishume:masterfrom
Enselic:thiserror
Feb 16, 2022
Merged

Implement Error and Display for all error enums by using thiserror#420
trishume merged 2 commits into
trishume:masterfrom
Enselic:thiserror

Conversation

@Enselic

@Enselic Enselic commented Jan 17, 2022

Copy link
Copy Markdown
Collaborator

Also do some minor tweaks to the way errors are represented, and add basic integration tests for errors, to give a feel for how the errors behave. In general the backwards compatibility for clients should be good. As the docs for thiserror states (emphasis mine):

Thiserror deliberately does not appear in your public API. You get the same thing as if you had written an implementation of std::error::Error by hand, and switching from handwritten impls to thiserror or vice versa is not a breaking change.

I.e. the code I remove should be implemented by the new Error derive.

Also mark all error enums as #[non_exhaustive] so we can add new error variants without semver breakage in the future.

Fixes #94

FWIW, bat has been using thiserror for 5 months without any issues so far.

@Enselic
Implement Error and Display for all error enums by using thiserror
2065ae6 
Also do some minor tweaks to the way errors are represented, and add basic
integration tests for errors, to give a feel for how the errors behave.

Fixes trishume#94
@Enselic
Mark all error enums as #[non_exhaustive]
120e1b3 
So we can add new error variants without semver breakage.
@Enselic Enselic mentioned this pull request Jan 17, 2022
Merged
@Enselic

Enselic commented Feb 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@trishume Hi, just a heads-up that this soon has been open for more than a month, and as per #382 (comment) I plan to merge this early next week.

Let me know if you want more time for review, that is no problem at all.

I am comfortable to merge this without review though because I consider it "obviously an improvement", and the changes to the public API are small, and when there is a change, it is obvious for clients what to do.

In fact, I've been working on a tool lately to diff changes to public APIs of Rust libraries. Here are the exact changes this PR does to the public API:

cargo install cargo-public-items

git checkout origin/master
cargo public-items > /tmp/before-PR

git checkout PR
cargo public-items > /tmp/after-PR

diff -U0 /tmp/before-PR /tmp/after-PR | grep -v @@

which gives this output (manually cleaned up a bit)

-pub enum variant syntect::LoadingError::ParseSyntax(ParseSyntaxError,Option<String>)
+pub enum variant syntect::LoadingError::ParseSyntax(ParseSyntaxError,String)

-pub fn syntect::LoadingError::cause(&self) -> Option<&Error>
+pub fn syntect::LoadingError::source(&self) -> std::option::Option<&std::error::Error + 'static>

+pub fn syntect::highlighting::ParseThemeError::source(&self) -> std::option::Option<&std::error::Error + 'static>
+pub fn syntect::highlighting::ParseThemeError::fmt(&self, __formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result
+pub fn syntect::highlighting::ParseThemeError::to_string(&self) -> String

+pub fn syntect::highlighting::SettingsError::fmt(&self, __formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result
+pub fn syntect::highlighting::SettingsError::to_string(&self) -> String

+pub fn syntect::parsing::ParseScopeError::fmt(&self, __formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result
+pub fn syntect::parsing::ParseScopeError::to_string(&self) -> String

-pub fn syntect::parsing::ParseSyntaxError::cause(&self) -> Option<&Error>
+pub fn syntect::parsing::ParseSyntaxError::source(&self) -> std::option::Option<&std::error::Error + 'static>

+pub fn syntect::parsing::ParseSyntaxError::from(source: ScanError) -> Self

each line represents a public item in the syntect API. - means "removed" and + means "added".

So the deprecated Error::cause has been replaced with Error::source, an Option<String> has been changed into an String. Apart from that there are only additions to the API (implementations of Display and Error).

Disclaimer: The tool is still work in progress so I would not trust my life with it, but it gives a good indication at least.

trishume
trishume approved these changes Feb 16, 2022
View reviewed changes

@trishume trishume left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Great PR! Sorry for leaving it for so long, this would indeed have been totally fine to merge on your own after a month. I moved cities and started a new job this month so I've been busy.

@trishume trishume merged commit f51e41b into trishume:master Feb 16, 2022
@Enselic Enselic deleted the thiserror branch February 23, 2022 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trishume trishume trishume approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement Error trait for error types

2 participants

@Enselic @trishume