Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-13 Thread Benjamin Mahler
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

Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-10 Thread Neil Conway
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

Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-09 Thread Cong Wang
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

Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-08 Thread Alex R
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

Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-07 Thread haosdent
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

Re: mesos git commit: Replaced CHECK with CHECK_READY.

2016-05-07 Thread 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