Closed steveej closed 4 years ago
@lucab thanks a lot for the thorough review. I addressed all the comments. Please take another look.
No further remarks from my side. I'll wait for the squash+rebase at https://github.com/camallo/dkregistry-rs/pull/138#issuecomment-576343665 to happen and then do the final stamp on this.
Thanks for the review @schrieveslaach!
The tests could be made simpler by using
tokio::test
but that should not block the PR.
I gave it a quick try. I would have had to give it more thought than I want to for the scope of this PR, when refactoring the tests which test the impl Stream
functions in the mock tests. I'll be happy to do a follow-up PR with that refactor. I want to give you and @lucab another chance to express any strong feelings about blocking the PR on this, otherwise I think we're good to merge it.
A follow-up PR sounds good to me.
LGTM, thanks both of you!
@schrieveslaach @steveeJ does any of you need this to be part of a release in the short term, or can we keep going and lazily tag one later on?
Would be nice if I could use a release(-candidate) in aixigo/PREvant#28 but you don't have to hurry.
This is based on #138 (thanks to @schrieveslaach!).
I'm happy to squash the new commits into the one's made by @schrieveslaach if we reach consensus on doing so.