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 :)
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.
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
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 :)