3 ms·

How often is commit bad enough to require revert, yet made it through testing and got released?

btown 9y ago
Code style, inconsistent APIs, a feature that's barely tested by its own contributor, introducing dependencies on innards that were planned to be removed by an ongoing refactoring project, etc. So many ways contributions can go wrong.
- diggan 9y ago
  All those things should and hopefully would have been caught when the PR was made, not after the commits made it into master.
  - btown 9y ago
    The entire context of this thread is that adding committers can introduce risk in the quality of accepted contributions. Building a trusted network of gatekeepers is hard.
    - nextlevelwizard 9y ago
      Just because stuff gets to master doesn't mean they get to the release.
- snakeanus 9y ago
  > Code style How is that an issue in this day and age? Just run astyle or indent.
  - gopalv 9y ago
    > Just run astyle or indent. "Style" is more than indentation - my favourite example is something I worked with very closely, but always needed a cheat-sheet on my desk - the PHP arrays api. Just look at the docs for array_search and array_replace. This is much worse in managed memory languages - do you allocate memory, who frees it, who can use a buffer in zero-copy mode, who can't ... whether the functions are always re-entrant. All that said - any originating coder who burns out due to support issues, killing the community is measurably worse off than someone who has thousands of users who demand that you support such "legacy" APIs for another six months.
zzzeek 9y ago
I'm talking here about a developer API that is extremely flexible and extensible. Even one-liner contributions can have bad effects, and virtually all contributions I receive "work" at a superficial level and if they provide tests the tests usually pass. "made it through testing" is a low bar, the bigger implications of a patch need to be considered. Examples of PRs that "work" exactly as intended yet are the wrong way to solve the problem: https://github.com/zzzeek/sqlalchemy/pull/381 https://github.com/zzzeek/sqlalchemy/pull/381 https://github.com/zzzeek/sqlalchemy/pull/382 https://github.com/zzzeek/sqlalchemy/pull/382 Here's a large PR: https://github.com/zzzeek/sqlalchemy/pull/365 https://github.com/zzzeek/sqlalchemy/pull/365 that after lots and lots of back and forth I finally merged with lots of edits from me at: https://github.com/zzzeek/sqlalchemy/commit/7d3da6f850dca54b941275279470c37bec64a48f https://github.com/zzzeek/sqlalchemy/commit/7d3da6f850dca54b... ...aannnnndddd - after all that work and testing, it was wrong! Fortunately someone caught the mistake within the beta release: https://bitbucket.org/zzzeek/sqlalchemy/issues/4072/mysqldmlinsertvalues-shadows-the https://bitbucket.org/zzzeek/sqlalchemy/issues/4072/mysqldml... if I had released that fully and the whole world coded to "mysql.insert.values.bar == 5", that's broken API since it breaks the values() method. Backwards-incompatible fix required.

Code style, inconsistent APIs, a feature that's barely tested by its own contributor, introducing dependencies on innards that were planned to be removed by an ongoing refactoring project, etc. So many ways contributions can go wrong.

diggan 9y ago
All those things should and hopefully would have been caught when the PR was made, not after the commits made it into master.
- btown 9y ago
  The entire context of this thread is that adding committers can introduce risk in the quality of accepted contributions. Building a trusted network of gatekeepers is hard.
  - nextlevelwizard 9y ago
    Just because stuff gets to master doesn't mean they get to the release.
snakeanus 9y ago
> Code style How is that an issue in this day and age? Just run astyle or indent.
- gopalv 9y ago
  > Just run astyle or indent. "Style" is more than indentation - my favourite example is something I worked with very closely, but always needed a cheat-sheet on my desk - the PHP arrays api. Just look at the docs for array_search and array_replace. This is much worse in managed memory languages - do you allocate memory, who frees it, who can use a buffer in zero-copy mode, who can't ... whether the functions are always re-entrant. All that said - any originating coder who burns out due to support issues, killing the community is measurably worse off than someone who has thousands of users who demand that you support such "legacy" APIs for another six months.

zzzeek 9y ago

I'm talking here about a developer API that is extremely flexible and extensible. Even one-liner contributions can have bad effects, and virtually all contributions I receive "work" at a superficial level and if they provide tests the tests usually pass. "made it through testing" is a low bar, the bigger implications of a patch need to be considered. Examples of PRs that "work" exactly as intended yet are the wrong way to solve the problem: https://github.com/zzzeek/sqlalchemy/pull/381 https://github.com/zzzeek/sqlalchemy/pull/381 https://github.com/zzzeek/sqlalchemy/pull/382 https://github.com/zzzeek/sqlalchemy/pull/382 Here's a large PR: https://github.com/zzzeek/sqlalchemy/pull/365 https://github.com/zzzeek/sqlalchemy/pull/365 that after lots and lots of back and forth I finally merged with lots of edits from me at: https://github.com/zzzeek/sqlalchemy/commit/7d3da6f850dca54b941275279470c37bec64a48f https://github.com/zzzeek/sqlalchemy/commit/7d3da6f850dca54b... ...aannnnndddd - after all that work and testing, it was wrong! Fortunately someone caught the mistake within the beta release: https://bitbucket.org/zzzeek/sqlalchemy/issues/4072/mysqldmlinsertvalues-shadows-the https://bitbucket.org/zzzeek/sqlalchemy/issues/4072/mysqldml... if I had released that fully and the whole world coded to "mysql.insert.values.bar == 5", that's broken API since it breaks the values() method. Backwards-incompatible fix required.