> On May 21, 2016, at 7:25 PM, Clayton Daley <clayton.da...@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@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
