Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1036
I want to take a step back a moment here. I think there are some useful,
clarifying refactorings in this PR. I'm generally in favor of refactoring
improvements e.g. "handleTick(tuple)" that sum up the code nicely. I also like
adding sensible clarifying comments and making tests more clear. However, I'm
not sure how I feel about mixing refactoring like this with the bug fix itself.
It seems to me that the actual bug fix is roughly 5-10 lines of code. It makes
it more difficult to determine code that's specific to the change in mind. I
noticed this while reviewing https://github.com/apache/metron/pull/1015 as
well. Might it be better to do separate refactoring PR's? I am certainly not
suggesting we be pedantic about it - I'm not even clear how I would even
suggest a reasonable rule to follow, to be honest. Part of me thinks "ok,
broken window theory and boyscout rules - this is great stuff." But in general,
I think that splitting the refactoring will allow reviewers to voice opinio
ns specific to coding style and refactoring changes as distinct from a feature
or bug fix itself. It's clear that we all have varying opinions on what reads
best in our code, and I think splitting those discussions off will make the
review process more expedient.
---
