-
Notifications
You must be signed in to change notification settings - Fork 124
Inline oid module #238
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
Inline oid module #238
Conversation
rcgen/src/lib.rs
Outdated
use crate::key_pair::PublicKeyData; | ||
pub use crate::key_pair::{KeyPair, RemoteKeyPair}; | ||
use crate::oid::*; | ||
mod csr; |
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 it's nicer before the reorderings.
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.
Which part do you prefer? Separating use
from mod
? The main confusing thing (and unexpected) for me was that the module declarations are like halfway through the lib.rs
module.
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.
Changed this so it only moves the mod
declarations up and removes the unnecessary crate::
prefixes from the imports.
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.
separating use from mod?
Yeah that separation is useful I think.
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.
So do you want to approve this in its current state?
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.
Done, approved.
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.
Thanks!
There is merge conflicts, needs a rebase before merging. |
Following up on some suggestions from #237 (comment).