Skip to content

New error type #2500

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

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

New error type #2500

wants to merge 23 commits into from

Conversation

aumetra
Copy link
Member

@aumetra aumetra commented Jun 16, 2025

Now with 99% less enum cases. Add that to the cerial packaging as a selling point.

Closes #2469

@aumetra aumetra requested a review from chipshort June 16, 2025 13:43
@aumetra
Copy link
Member Author

aumetra commented Jun 16, 2025

Also the change on the shell files is very smol (just replacing /bin/bash with /usr/bin/env bash because if my NixOS install would've acted up one more time because of a missing interpreter, I would've gone insane)

@aumetra
Copy link
Member Author

aumetra commented Jun 17, 2025

Please I literally let the whole test suite run locally, how were these errors not caught
4x

@chipshort
Copy link
Collaborator

Please I literally let the whole test suite run locally, how were these errors not caught

Even the ./devtools/check_contracts_full.sh? Looks like what's failing are the contracts' integration tests.

@aumetra
Copy link
Member Author

aumetra commented Jun 17, 2025

Even the ./devtools/check_contracts_full.sh? Looks like what's failing are the contracts' integration tests.

Probably typo'd when executing. Taking care of it.

Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Very neat that we can simplify the error type like that!

#!/bin/bash
#!/usr/bin/env bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a fellow NixOS user (not on the work laptop though), I salute you 🫡

Std(#[from] StdError),
Std(StdError),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not use #[from] anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because of the requirement of thiserror. If you want to use the #[from] feature, it has to implement core::error::Error + Send + Sync (which we can't fulfill because of trait blanket impls; same reason why anyhow can't implement the error trait)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's a bit sad. What if we used manual (or macro assisted) impls for all our error types instead of the blanket impl?

Comment on lines +114 to +116
// "mom, can we have specialization?"
// "we have specialization at home"
// specialization at home:
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆

Copy link
Collaborator

@chipshort chipshort 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! Just a changelog entry needed

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.

Redesign StdError type
2 participants