[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [DISCUSS] Vetoing commits

First off, thanks for staying engaged with this conversation, Vladimir. I know this stuff is uncomfortable to talk through.

On 8/10/18 1:34 PM, Vladimir Sitnikov wrote:
Josh>That doesn't mean you can stop CALCITE-2438 from happening meanwhile

Of course I can't.

Ok, good. Just making sure :)

What was the better way to indicate that "changes to core functionality
requires tests"? All the added tests cover just positive scenarios.
What was the better way to indicate that "error messages should explain the
nature of the failure"? If course it might be not required at all (e.g.
when test has single assertion, and the name of the tests is meaningful),
however 2348 includes multiple asserts in a single test method, so it is
important to know which failed.

Should have I said "+1, but please add negative tests and add appropriate
error messages"?
No, I don't think you should feign approval of something which you don't think is good! That said, this seems to have spiraled out of control because instead of asking for a change to be made, you levied a veto. I think your action was just too heavy for the situation at hand.

For example, even if Julian/whoever had committed the patch in that state (without negative tests), it doesn't immediately hurt anything. An amendment to that commit can easily be done later without directly affecting the quality of the project (meaning, it would not cause any other developer grief if they pulled an update. not saying that the lack of test-coverage is OK).

In short, I think using a lighter-touch here would have gone a long way to prevent this conflict in the first place :)