That makes sense, if the summary of the change was "Style cleanups within
..." rather than "Replaced CHECK with CHECK_READY" then doing an overall
style sweep of the file together sounds ok to me. Here the reviewer will be
more likely to look through the file for other style changes that are
Hi Ben,
Thanks for raising this!
My thinking for grouping the changes together in a single review is
basically what AlexR said: I agree with doing "one thing per patch",
but I felt like a header cleanup was sufficiently close to CHECK
cleanup that they could be grouped together. If there's
On Sun, May 8, 2016 at 1:28 AM, Alex R wrote:
> I agree that "atomic patches" (those that do one thing per patch) are a
> good thing because they simplify navigating history, do blame and bisect.
> But how to define that "one thing"? Some people would say, that a new
If "one
I agree that "atomic patches" (those that do one thing per patch) are a
good thing because they simplify navigating history, do blame and bisect.
But how to define that "one thing"? Some people would say, that a new
feature is one thing, and if introducing a feature requires some
refactoring, it
I notice we sweep update the code which the style is incorrect. For
example, replace "A< >" with "A<>", replace ".get()" to "->", remove
incorrect space, adjust line length. Does this mean we need to split them
out as an additional patch?
On Sun, May 8, 2016 at 8:38 AM, Benjamin Mahler
Hm.. any reason that unrelated headers were touched and the using statement
was removed in this patch?
My concern with mixing unrelated changes within a single patch is that
patches become less precise. If one reads the patch there is additional
overhead in understanding what is related to the