-
Notifications
You must be signed in to change notification settings - Fork 52
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
Name iterator #140
Name iterator #140
Conversation
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
==========================================
- Coverage 96.28% 96.01% -0.28%
==========================================
Files 16 16
Lines 4070 4061 -9
==========================================
- Hits 3919 3899 -20
- Misses 151 162 +11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
) | ||
.find_map(|result| { | ||
let name = match result { |
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 downside of the stricter iterator chaining is that each of the NameIterator
users have to deal with error values separately (rather than iterate_names()
propagating them in one place). However, since the only thing they have to do is yield them to the find_map()
iterator, that doesn't seem like a big issue.
_ => { | ||
// mismatch between constraint and name types; continue with current | ||
// name and next constraint | ||
continue; |
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.
feels a bit less readable because we have this continue
in a two-deep loop (though the comment gives a hint of which loop it applies to if the reader missed the inner loop), and the loop bodies are quite long now - with load-bearing code that has to run after the inner loop completes. perhaps hoist fn general_subtree
out of the loop body? or drop the previous DirectoryName
commit?
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.
not a hard objection either way, it's a bit subjective.
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 hoisted general_subtree()
out of the loop body, hopefully that helps a bit.
I think there's more that can be done here, but probably still makes sense to merge this first.
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 looks good to me 👍 Thanks djc
This makes the abstractions a lot more intuitive/idiomatic in my opinion.
I think it has the potential to bring
list_cert_dns_names()
out of thealloc
domain (see #46), but that would be at the cost of making it yield animpl Iterator<Item = Result<GeneralName<'_>, Error>>
rather than aResult<impl Iterator<Item = GeneralName<'_>>, Error>
which might be undesirable?