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

Add custom error types to the Decode and DecodeValue traits. #1055

Merged
merged 1 commit into from
Mar 2, 2024
Merged

Add custom error types to the Decode and DecodeValue traits. #1055

merged 1 commit into from
Mar 2, 2024

Conversation

turbocool3r
Copy link
Contributor

Adds support for specifying custom error types for the Encode, Decode, EncodeValue, DecodeValue, DerOrd and ValueOrd traits (see #1053).

@tarcieri
Copy link
Member

tarcieri commented May 8, 2023

Just to set expectations: it will probably be a few months before we're ready to start making breaking changes again

tarcieri
tarcieri reviewed May 8, 2023
View reviewed changes
der/src/ord.rs Outdated Show resolved Hide resolved
@turbocool3r
Copy link
Contributor Author

Just to set expectations: it will probably be a few months before we're ready to start making breaking changes again

That's ok, I'll leave this PR in place until it can be accepted.

@turbocool3r turbocool3r changed the title Add custom error types to the Encode/Decode and (Der|Value)Ord traits. Add custom error types to the Decode and DecodeValue traits. May 8, 2023
@tarcieri
Copy link
Member

tarcieri commented Jan 21, 2024 •
edited
Loading

It's possible to make breaking changes now.

However, I've opened #1328 which provides a less invasive alternative: it allows der::Error to carry with it an arbitrary error source when the std feature is enabled. It's not perfect but at least provides a stopgap solution.

That said, I think this approach is still interesting, and closer to e.g. what serde does.

@turbocool3r turbocool3r mentioned this pull request Jan 22, 2024
Merged
@tarcieri
Copy link
Member

tarcieri commented Jan 22, 2024 •
edited
Loading

@turbocool3r if you can get this PR green we can consider it over #1328.

@turbocool3r
Copy link
Contributor Author

Ok, I'll try to do this by Wednesday

@turbocool3r turbocool3r force-pushed the custom-errors branch 7 times, most recently from 1ff2bf5 to 5638fc3 Compare January 23, 2024 21:13
@turbocool3r
Copy link
Contributor Author

It seems this won't be completely green because there are some dependencies on external crates like signature which should be modified separately.

There also was an issue with the DecodePem trait: adding a generic error type there causes the from_pem method to fail the borrow check even if both T and Error are marked as 'static. Forcing the error to be der::Error there fixes the issue.

@tarcieri
Copy link
Member

It seems this won't be completely green because there are some dependencies on external crates like signature which should be modified separately.

Err, how so?

@turbocool3r
Copy link
Contributor Author

Like here https://github.com/RustCrypto/formats/actions/runs/7631920556/job/20790968317?pr=1055#step:4:297.

@tarcieri
Copy link
Member

Aah, in ecdsa. I can open a PR for that if you'd like.

@turbocool3r
Copy link
Contributor Author

Aah, in ecdsa. I can open a PR for that if you'd like.

It would be nice. I think we need to first get this as it goes to see the complete list of crates that need to be modified.

@turbocool3r
Copy link
Contributor Author

Yes, it seems it's only ecdsa, not signature.

@turbocool3r turbocool3r marked this pull request as ready for review January 23, 2024 21:45
@turbocool3r
Copy link
Contributor Author

Hey, how about that pull request? I could probably do that myself to save some time.

@tarcieri
Copy link
Member

Sorry, I've been meaning to get to it and I'd love to land this, but if you'd like to go for it that works too.

It's going to be oddly circular so you need to make a PR to https://github.com/rustcrypto/formats which uses a [patch.crates-io] directive to point to this branch, then you'll need to reference the branch in this PR to complete the cycle.

@turbocool3r
Copy link
Contributor Author

Did you mean the https://github.com/RustCrypto/signatures repo? By this branch do you mean my fork or some other branch?

@tarcieri
Copy link
Member

Err sorry, yes https://github.com/RustCrypto/signatures

You'll need to point to your branch on your fork for both this repo when updating signatures and again in a circular manner in your fork of signatures to update this PR

turbocool3r added a commit to turbocool3r/signatures that referenced this pull request Feb 14, 2024
@turbocool3r
Add support for custom error types in der.
d9d394a 
Goes in pair with [this](RustCrypto/formats#1055) PR to `formats`.
@turbocool3r turbocool3r mentioned this pull request Feb 14, 2024
Merged
@turbocool3r
Copy link
Contributor Author

Seems all green now, the signatures PR is RustCrypto/signatures#809.

@tarcieri
Copy link
Member

Thanks, will review and try to get this merged ASAP

@turbocool3r
Copy link
Contributor Author

Thanks!

baloo
baloo reviewed Feb 14, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@tarcieri tarcieri merged commit c501837 into RustCrypto:master Mar 2, 2024
165 checks passed
@turbocool3r turbocool3r deleted the custom-errors branch March 3, 2024 07:28
@turbocool3r turbocool3r restored the custom-errors branch March 3, 2024 16:26
turbocool3r added a commit to turbocool3r/signatures that referenced this pull request Mar 3, 2024
@turbocool3r
Add support for custom error types in der.
8620031 
Goes in pair with [this](RustCrypto/formats#1055) PR to `formats`.
tarcieri pushed a commit to RustCrypto/signatures that referenced this pull request Mar 3, 2024
@turbocool3r
dsa+ecdsa: support der custom error types (#809)
31838b9 
Goes with this `formats` PR: RustCrypto/formats#1055
@turbocool3r turbocool3r deleted the custom-errors branch March 6, 2024 17:29
tarcieri added a commit that referenced this pull request Mar 28, 2024
@tarcieri
Revert "der: add Error::(set_)source; remove Clone + Copy (#1328)"
c4a2a02 
This reverts commit 7784f2d.

This ended up complicating downstream error types, particularly in
`no_std` contexts.

It's also not really necessary now that #1055 has landed.
@tarcieri tarcieri mentioned this pull request Mar 28, 2024
Merged
tarcieri added a commit that referenced this pull request Mar 28, 2024
@tarcieri
Revert "der: add Error::(set_)source; remove Clone + Copy (#1328)…
64ca37f 
…" (#1371)

This reverts commit 7784f2d.

This ended up complicating downstream error types, particularly in
`no_std` contexts.

It's also not really necessary now that #1055 has landed.
baloo added a commit to baloo/formats that referenced this pull request Mar 28, 2024
@baloo
der: fixup windows write_secret_file helper
63fd014 
This signature was missed in RustCrypto#1055
@baloo baloo mentioned this pull request Mar 28, 2024
Merged
baloo added a commit that referenced this pull request Mar 28, 2024
@baloo
der: fixup windows write_secret_file helper (#1372)
c03c066 
This signature was missed in #1055
@tarcieri tarcieri mentioned this pull request Aug 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baloo baloo baloo left review comments

@tarcieri tarcieri tarcieri 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.
3 participants
@turbocool3r @tarcieri @baloo