-
Notifications
You must be signed in to change notification settings - Fork 26
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
Update dependencies & new release #218
Conversation
039d755
to
887a3aa
Compare
### Breaking Changes | ||
|
||
- Minimum supported Rust version updated to 1.71 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env_logger
apparently bumped their MSRV and was causing issues when built on lower versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's only a dev dep, I am not sure we should bump Marv, but rather stick to 0.10.x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 We certainly could. I figured a 7 months old version was still at least reasonable
#[cfg(feature = "signature-pgp")] | ||
pub fn signature_key_id(&self) -> Result<Option<String>, Error> { | ||
pub fn signature_key_ids(&self) -> Result<Vec<String>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rpgp
changed the underlying API here to reflect that signatures can apparently have more than one issuer / key ID.
} | ||
} | ||
result | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some logic got removed, could you explain a bit why it's not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, it got moved down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about msrv, otherwise LGTM
📜 Checklist
--all-features
enabled