Concept ACK - Agree with the idea and overall direction, but haven't reviewed the code changes or tested them.
utACK (untested ACK) - Reviewed and agree with the code changes but haven't actually tested them.
Tested ACK - Reviewed the code changes and have verified the functionality or bug fix.
ACK - A loose ACK can be confusing. It's best to avoid them unless it's a documentation/comment only change in which case there is nothing to test/verify; therefore the tested/untested distinction is not there.
NACK - Disagree with the code changes/concept. Should be accompanied by an explanation.
Yes, this works for me.
As for merging, I'm always very grateful when Ben does it -- though I agree that it would make more sense for me to do it when I can test-then-merge.
Thanks,
Richard
> On Sep 13, 2017, at 10:29 AM, Ben Gamari <ben@smart-cactus.org> wrote:
>
> Simon Marlow <marlowsd@gmail.com> writes:
>
>> On 19 August 2017 at 03:56, Richard Eisenberg <rae@cs.brynmawr.edu> wrote:
>>
>>> Hi devs,
>>>
>>> When reviewing a diff on Phab, I can "accept" or "request changes".
>>> Sometimes, though, I want to do both: I suggest very minor (e.g., typo)
>>> changes, but then when these changes are made, I accept. I'm leery of
>>> making the suggestions and saying "accept", because then someone working
>>> quickly may merge without noticing the typos. Does Phab have such an option?
>>>
>>
>> "Accept with nits" is standard practice, but you're right it can go wrong
>> when someone else is merging accepted diffs. We could adopt a standard
>> comment keyword, e.g. "NITS" that indicates you'd like the nits to be fixed
>> before committing, perhaps?
>>
> Sounds reasonable to me.
>
>
>> Also, I don't think it's a good idea to merge commits when the author is a
>> committer, they can land themselves.
>>
> I would be quite happy to not have to merge such patches; I merely merge
> them currently since I thought it was generally expected.
>
> On the other hand, I generally do integration builds on the batches of
> patches that I merge which can sometimes catch validation issues.
> However, I expect this will be less of an issue with the
> test-before-merge support in the pipeline.
>
> Cheers,
>
> - Ben
>
_______________________________________________
ghc-devs mailing list
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs