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 Display trait on CoseError enum. #45

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

scouten-adobe
Copy link
Contributor

Resolves #44.

Copy link
Collaborator

@daviddrysdale daviddrysdale 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, only minor things to mention.

Also, could you add something like the following to CHANGELOG.md please:

## 0.3.1 - TBD

- Implement `Display` for `CoseError`.

src/common/mod.rs Outdated Show resolved Hide resolved
src/common/mod.rs Outdated Show resolved Hide resolved
src/common/tests.rs Outdated Show resolved Hide resolved
src/util/mod.rs Outdated
);
match result {
Ok(_) => {
panic!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is failing a CI check because we've got a local convention that things that panic! need a comment of the form "safe: ..." on the same line to document why it's OK to panic. (Unfortunately the check is very primitive, so it doesn't notice that this is #[cfg(test)] code.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, it's an even dumber check than that :-( (./scripts/check_format.sh).

Let me see if I can make it less pesky…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting challenge. If the panic! spreads across multiple lines, cargo fmt wants to put the comment on a line of its own.

Might be simpler for me to rewrite this as an assert!(false...).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, cargo fmt and check_format.sh fight each other.

Shifting to assert should work though … the check is just for panic and the red things on https://tinyurl.com/rust-transform.

@daviddrysdale daviddrysdale merged commit f9f7034 into google:main Feb 2, 2022
@daviddrysdale
Copy link
Collaborator

Thanks!

@scouten-adobe scouten-adobe deleted the error-impl-display branch February 2, 2022 19:02
falko17 pushed a commit to namib-project/coset that referenced this pull request Feb 16, 2022
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.

Please implement Display trait on CoseError type
2 participants