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.

Sign up for GitHub

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

Jump to bottom

Use thiserror #211

Merged
merged 1 commit into from
Dec 22, 2021
Merged

Use thiserror #211

merged 1 commit into from
Dec 22, 2021

Conversation

madig
Copy link
Collaborator

@madig madig commented Dec 16, 2021 •
edited
Loading

An experiment to see how using https://docs.rs/thiserror/latest/thiserror/ would look like.

Closes #204

@madig madig force-pushed the error-tweaks-thiserror branch 2 times, most recently from 47f663c to f283a3b Compare December 17, 2021 13:35
@madig madig mentioned this pull request Dec 17, 2021
Closed
@madig madig linked an issue Dec 17, 2021 that may be closed by this pull request
Errors: use https://lib.rs/crates/thiserror to reduce description duplication? #204
Closed
@madig
Copy link
Collaborator Author

madig commented Dec 17, 2021

I suppose there are a few more errors in other modules that could be thiserror'ified if this is the approach we'd like.

Base automatically changed from error-tweaks to master December 17, 2021 19:55
cmyr
cmyr reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I'm happy to see this merged, I took a few quick measurements and compile time does not seem to be significantly impacted.

src/write.rs Outdated Show resolved Hide resolved
@linebender linebender deleted a comment from github-actions bot Dec 21, 2021
@github-actions
Copy link

🗜 Bloat check ⚖️

Comparing b33d57f against df9f486

target old size new size difference
target/release/examples/load_save 1.9 MB 1.9 MB -376 Bytes (-0.02%)
target/debug/examples/load_save 8.55 MB 8.56 MB 3.69 KB (0.04%)

@madig madig marked this pull request as ready for review December 21, 2021 22:40
cmyr
cmyr approved these changes Dec 22, 2021
View reviewed changes
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

thanks!

@madig madig merged commit 6816795 into master Dec 22, 2021
@madig madig deleted the error-tweaks-thiserror branch December 22, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmyr cmyr cmyr 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.

Errors: use https://lib.rs/crates/thiserror to reduce description duplication?
2 participants
@madig @cmyr