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

Implement Error for OidParseError #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MattesWhite
Copy link

While using your excellent crate I noticed that OidParseError does not implement Error. I can see that in no_std environments this is not required but for many cases this is a low-hanging nice-to-have.

This PR implements the Error trait for std builds.

@MattesWhite MattesWhite force-pushed the matteswhite/impl-error-for-oidparseerror branch from 31fd707 to 72c81a2 Compare April 23, 2024 14:18
Comment on lines 24 to 25
#[cfg_attr(feature = "std", error("Relative OIDs must not be empty"))]
RelativeTooShort,
Copy link
Contributor

@cpu cpu Apr 23, 2024

Choose a reason for hiding this comment

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

Could this be implemented without adding the new variant? It's a semver breaking change with this approach and it feels like the value of the separate RelativeTooShort variant might not justify it. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

Right I forgot to keep semver in mind. Of course we don't need to introduce a breaking change for this.

Fixed it.

Only available with 'std' feature.
@MattesWhite MattesWhite force-pushed the matteswhite/impl-error-for-oidparseerror branch from 72c81a2 to 5bea3e1 Compare April 23, 2024 14:20
@MattesWhite
Copy link
Author

So is there anything else I can do to finish/merge this PR?

Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

@chifflier I think this is ready for review if you have a chance. It seems like a reasonable diff to me.

@chifflier chifflier self-assigned this May 6, 2024
@chifflier
Copy link
Member

Hi,
Thank you for your contribution. Before considering merge (PR looks nice in itself), I considered how other errors are derived in this crate, in file src/error.rs.

Previously, this crate was doing the same, but ended up choosing displaydoc to implement errors in ba72bde (see also dtolnay/thiserror#64 (comment)).
It is a bit annoying, because thiserror brings more features than just Display, but displaydoc integrates nicely with documentation since it uses regular doc comments. It is still used for SerializeError.

At this point, I am unsure what solution is best. Any thoughts on this?

@MattesWhite
Copy link
Author

The real value of thiserror shows when you start to have nested errors. Because thiserror does not only do the empty implementation of std::error::Error but with the #[from] or #[source] annotation it also implements a custom version of Error::source() which returns the inner error, allowing users to inspect the whole error chain. Admittedly, this is not used for OidParseError but you could in std build which would require a lot of conditional compilation shenanigans. This is also the reason why I didn't do it.

displaydoc is interesting as it's first example literally shows how it interacts with thiserror. It seems that thiserror doesn't require the #[error(...)] annotations when the underlying type already implements Display. I change the PR accordingly.
So all in all I came to the following conclusion:

  1. For no-std error types displaydoc is totally fine
  2. #[cfg_attr(feature = "std", derive(thiserror::Error))] on those errors for std builds should result in good enough std::error::Error types

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