[Twisted-Python] overview: new review queue venue

Sun May 22 07:18:21 MDT 2016

>
> The thing is, if you perceive it as "hostile" that a project closes a PR -
> i.e. "says that they're not going to do more work on it" - that is a
> cultural problem; it suggests a certain implicit level of passive
> aggression in opening a PR which I don't want to assume.  It's sort of like
> having a culture where you can just send anybody an email asking them to do
> whatever and it would be "hostile" for them to refuse.  In such a culture
> people don't say "yes", but they do start to ignore messages  Closing the
> PR is a more accurate reflection of *reality* - the project (twisted) is
> not going to do anything about the PR in its current state, so it shouldn't
> be left open.  It also clearly demarcates the completion of a review.
>
...

I'd much rather our new contributors get a little confused about the
> culturally unusual step of closing a PR than to have their work be
> accidentally but systematically discriminated against in favor of people
> who know how to bug the right people in IRC or email.
>

It's your party, but I think this vastly undervalues first impressions in
OSS engagement.  From all the projects I've contributed to on Github -- yes
technically Github not Git though Bitbucket has equivalent features (and
unlimited private repos for free!) -- closing means "this is off the
table".  It's a nice clear signal that there's nothing the contributor can
do to fix it -- e.g. I had an LSP-valid change that passed all tests run
afoul of some obscure PHP method signature limitation when mixed with other
packages.

I'll wager only the active contributors will remember the proposed subtlety
a month from now.  As a big break from Github norms, It's going to hit
everyone else at the worst time... a first PR submission.  That's the
moment when a contributor is trying to get a sense of the culture of the
team that manages the project.  They're at their most vulnerable and
violating *their* norms significantly increases the odds that they'll leave
the fix in a private fork and disengage.

Twisted operates at a different level so this may not be a bad thing.  You
may benefit from actively discouraging dabblers -- especially given
resource constraints.  But there aren't going to be a lot of "first PR"
folks on this list to point out the effect of this break from norms.

Clayton Daley

On Sun, May 22, 2016 at 12:12 AM, Glyph <glyph at twistedmatrix.com> wrote:

>
> On May 21, 2016, at 7:25 PM, Clayton Daley <clayton.daley at gmail.com>
> wrote:
>
> To qualify my comments, I've yet to contribute to Twisted because I don't
> have a good enough grasp of its internals, but I have contributed to a
> variety of Git-based OSS projects.  I definitely get uneasy with the
> general idea that we're trying to "replicate workflow A from Trac in
> tangentially related Git PR feature".
>
>
> The workflow is not "from Trac".  The instantiation in Trac is not optimal
> either, which is why I described the abstract desired workflow separately
> from our existing instantiation.
>
> We're in Git. We're hoping to solicit PRs from Git users. Git users will
> be used to the way PRs are used in other OSS Git projects.
>
>
> I think you mean "GitHub".  Git PRs don't work at *all* like GitHub PRs.
> :).
>
> Glyph has some valid criticisms of the situation in some projects, but it
> should still be the starting point. For example, closing a PR strikes me as
> a bad idea -- for lack of a better word, it feels "hostile" to me and
> certainly unwelcoming.
>
>
> The thing is, if you perceive it as "hostile" that a project closes a PR -
> i.e. "says that they're not going to do more work on it" - that is a
> cultural problem; it suggests a certain implicit level of passive
> aggression in opening a PR which I don't want to assume.  It's sort of like
> having a culture where you can just send anybody an email asking them to do
> whatever and it would be "hostile" for them to refuse.  In such a culture
> people don't say "yes", but they do start to ignore messages  Closing the
> PR is a more accurate reflection of *reality* - the project (twisted) is
> not going to do anything about the PR in its current state, so it shouldn't
> be left open.  It also clearly demarcates the completion of a review.
>
> People feel very differently about workflow, of course, but I've
> definitely heard from other OSS maintainers that the average workflow of
> volunteer projects often devolves into a huge backlog of un-reviewed stuff,
> which obscures the new stuff, and if you want something to actually get
> reviewed and move along you need to know the maintainers of the project and
> ask them personally.
>
> I'd much rather our new contributors get a little confused about the
> culturally unusual step of closing a PR than to have their work be
> accidentally but systematically discriminated against in favor of people
> who know how to bug the right people in IRC or email.
>
> In several of the projects I've seen, Git tags fill these roles. Piwik has
> a "needs review" tag -- the short list for reviewers. Looks like it's a
> manual add, but maybe it could be automated. Once reviewed, Piwik has tags
> like "Tests & QA". ZendFramework has a generic "awaiting author update".
>
>
> This was my original idea. The problem with GitHub labels ("Git tags" are
> something completely different) is that they can't be applied by external
> contributors.  You need write access to the repository to be able to
> manipulate them.  It's very important to our workflow that external
> contributors be able to re-submit their PRs.  We could have a bot for that
> (again, this was the original plan).  But it seems like using the open /
> closed state to reflect the we will do some work on this / we won't do any
> more work on this is actually closer to the "native" state of github.
>
> To address Glyph's concerns about lingering PRs, perhaps the combination
> of:
>
>    - A policy like "a reviewer must accept, close, or tag with one of the
>    next step tags"
>
> This doesn't address the shortcomings of labels, to wit, external
> contributors need the ability to manipulate them somehow, and if a PR isn't
> "in review" by default, then they have to whisper some magic comment to
> make anyone take a look at it.  If we use open/closed, then the default
> action of "open a PR" will cause someone to look at it, even if our bot is
> nonexistent or temporarily offline.
>
>
>    - A short list of common next steps like "code quality", "needs
>    tests", "second opinion", "not review ready"... plus a generic "other
>    author action"
>
> The next steps are always the same: "respond to the review".  The review
> may include any of these, any combination of these, or also possibly
> questions that the author must answer.  Formally separating these would be
> a weird tweak on our current workflow that I don't see helping.
>
>
>    - Auto-close tickets except those with "needs review" or "second
>    opinion" (say) 30 days after the last action.
>
> I think you mean "PRs", not "tickets"?  Issues (which are closer to trac
> "tickets") can remain open indefinitely.  I am not really in favor of any
> kind of auto-closing or expiration given that the project has wildly
> variable levels of resources depending on people's spare time; sometimes we
> have a whole bunch of reviewers active and can get to things within 24
> hours, sometimes we're severely overtaxed and can't look at anything for 6
> weeks.
>
> Drive-by comments on a PR are sometimes helpful, but should be used
>> sparingly.  Mostly, discussion should happen on *issues*, not PRs.  A PR
>> is a suggested resolution to a problem, and we might reject one solution,
>> but an issue should describe the problem itself.
>
>
> While an Issue is a good place for discussion about a problem, it lacks
> the reference code often included in a PR.  You can't ask "how about this
> approach" without showing the approach.  As an added bonus, most systems
> run travis on PRs so you get a sense of "this approach is thorough" or
> "this idea still breaks something".
>
>
> This is exactly why issues and PRs should be separated.  If you only have
> one artifact - the PR - to represent both the issue and the potential
> solution, then you can't get rid of the potential solution (reject the PR)
> without also getting rid of the description of the problem.  Github already
> automatically shows anywhere that your PR or issue is mentioned, so all you
> need to do to say "how about this approach" is to put the words "fixes
> #NNN" in your PR description.  As a bonus, that will make it automatically
> close the issue when the PR is merged.
>
> -glyph
>
> _______________________________________________
> Twisted-Python mailing list
> Twisted-Python at twistedmatrix.com
> http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160522/500d4777/attachment-0002.html>