Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The great rename plus cleanup. #488
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
The great rename plus cleanup. #488
Changes from 42 commits
3df971c
0bf028f
c09721b
d04cc9f
82b65ee
c2037fe
21cb3a8
cdfbc54
318ea4b
6cabdc4
c991ea2
e6f225d
7b4885b
6e5227d
7ba5b4f
5cfce9f
6be0156
4e5b755
1b40d27
e7b532a
3b6786e
3eeca20
eaf49fe
b7ecbcf
9f139b5
55cdb06
ce2ef08
8932a09
016ab54
c27c048
b8dac08
486b4b9
2e85e09
8609839
c465794
7764b95
feff956
097d1fe
dc21eea
138d5f2
da20ab1
2348d9d
5762aeb
d42be3f
4f95050
492db9d
96dee31
2fd6963
c53a21e
f76adfc
d1dd7b0
dc82bcc
89a011d
25e43ab
ef814a5
bbeb5f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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'm not sure that
Digest
needs to be anenum
over all the cryptographic backends. At the end of the day, it's just a(DigestType, Vec<u8>)
, right? Perhaps we should define it that way for simplicity.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.
It seems that that would require an extra Vec. No it just gets AsRef from the underlying ring or openssl type.
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.
This function is specific to DNSKEY records, but this module seems to handle cryptography outside the context of DNS. Would the
dnssec
module be more appropriate to handle it?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 want to keep crypto self contained and not depend on dnssec. So crypto uses the types from base and dnssec uses crypto.
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 see that this has only been moved so is not new, but looking at it I wonder are "invalid" and "malformed" really the same thing? Something can be well formed but still invalid, right? So maybe it would be better to make these consistent, in which case what is it: invalid or malformed?
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.
Maybe it is helpful to start a separate discussion about code that just got moved. The code that got moved has many different authors. Do we want have a discussion about all of that in this PR?
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.
As, in my opinion, excluding minor somewhat unrelated changes wouldn't simplify the review process, and this is a general "cleanup" PR, I'm in favour of taking any additional minor improvements at the same time in this PR.
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.
It is fine as long as there is no expectation that I will make the changes.