wx930910 commented on pull request #41: URL: https://github.com/apache/qpid-proton-j/pull/41#issuecomment-915669651
> I'm not sure I would actually consider this a great improvement overall. Its just as (if not more) verbose as the original but harder to understand from a simple glance, doing effectively the same thing but in a less trivially readable way. The use of BaseHandler in the original doesn't seem an issue given that is the way it was intended to be used (and remains used that way in other parts of the same test). There are other approaches the test could have originally taken to use a local variable for the count, which is what I might have done personally, but I would say the logic is trivial as-is and far from unclear even without doing so. Overall the Reactor is not a focus though either way and hasnt been for some time. > > Aside, the used imports should have been added rather than a wildcard, the comments being added dont seem necessary and are more of a distraction than a help, and the alignment is clearly way off for most of the changed lines, presumably from use of tabs vs spaces in the existing file. The commit log and PR title should have referenced the JIRA key to link them. Not necessarily an issue currently as I don't plan to merge this as is. @gemmellr Thanks for the comments! if we totally get rid of the variable `received` and use `Mockito.verify()` instead of assertion to check method execution status, will it make the test logic more explict? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
