> On Jan 29, 2024, at 10:36 AM, Adi Roiban <[email protected]> wrote:
>
> Hi
>
> I would like to propose a change to the contribution policy.
>
> The current policy is
>
> Authors: How to get your change reviewed
> There must be an issue in the Twisted Github Issue tracker describing the
> desired outcome.
>
> This is found in our docs at
> https://docs.twisted.org/en/latest/development/review-process.html#authors-how-to-get-your-change-reviewed
>
Personally I find it less burdensome to have a blanket policy ("file an issue
saying what the work is, then open a PR doing the work") than to have a complex
set of rules ("identity the type of your issue from this menu. does it match
this set of criteria? great. now let's evaluate a checklist to see if it needs
to follow process A, B, or C depending on the type of change that you are
interested in proceeding with.").
> I would like to have this rule to allow pull requests without a Github issue
> for the following cases:
> * typo fixes in docstrings
> * changes to documentation
> * changes that only affect the automated tests, and which don't change the
> production code.
My primary concern with all "no need for an issue" suggestions is that we will
get ill-considered changes where people have not thought about what the impact
of the change is properly.
With these specific proposed rules, "changes to documentation" is, in
particular, an extremely broad category that includes a ton of potentially
complex and difficult issues. "changes that only affect automated tests" could
similarly be large system-wide refactorings that comprise hundreds of lines. I
really want people to take a moment to write that down before they start on it.
I could get on board with something like this if it were "fewer than 5 lines of
changes" plus meeting one of these categories, though?
> In my opinion, asking for a separate GitHub each time you find a typo, is
> just red tape, and discourages small contributions.
This is a bit of a tangent, but whenever we talk about discouraging small
contributions, I feel like I need to raise the issue that what we need to
encourage is not contributions (we already have more than we can handle
<https://github.com/twisted/twisted/pulls?q=is:pr+is:open+label:needs-review>),
but code reviews. Reducing friction on contributions without reducing friction
on code reviews will just exacerbate this imbalance. Is the friction here
impeding your code reviews? This is a useful data point.
> With GitHub web UI, you can browse the current code, observe a type, fix the
> typo in the web and then send the changes as a PR.
> I feel that this is as frictionless as possible.
In general, we do not aspire to "frictionlessness". For example, the
requirement for a topfile is supposed to prompt the contributor to think about
the changelog every time, even if there is often nothing to put there. This is
intentional friction, and not something I want to get rid of.
That said, all friction must serve a purpose, and if you feel that this
requirement is not serving its purpose well, then I'm not totally opposed to
eliminating it or adding a category of exception. But since the very first
days of policy discussions on Twisted, people have been saying "well, sure,
but, like, trivial changes shouldn't require code review or unit tests, should
they?", usually followed immediately by the introduction of a huge regression
caused by an un-reviewed un-tested "trivial" change.
And this is why we have blanket policies in most cases rather than nuanced
specifics. Rather than frequently relitigate what a "trivial" change is, we
just say "100% code coverage is required on changed lines" and then only
discuss exceptions if they're causing a specific and difficult-to-resolve
problem, rather than just letting folks skip the process in the name of less
friction.
> A PR with a GitHub issue would still need a release note, but the release
> notes will have the PR ID, instead of the ticket ID.
Note that this creates a separate inefficiency; you can't create the news
fragment (as required by other policy) until you know the PR ID, which means at
least one full CI run gets kicked off that is inevitably going to fail. Better
to get the issue number first so you know what filename to use for the news
fragment, no?
> Here is an example of a PR https://github.com/twisted/twisted/pull/12088 that
> I consider a redtape.
>
> The PR description is just a placeholder `Fixes #12087`
> and then on Issue 12087 we have:
>
> Title: BufferingTLSTransport has docstring formatting errors. #12087
> Description: see
> https://docs.twisted.org/en/stable/api/twisted.protocols.tls.BufferingTLSTransport.html
>
> I think that it would help to have all this information just in the PR,
> instead of navigating between PR and Issue.
You are proposing a change to policy with arguments from the perspective of the
contributor, but your only example here is from the perspective of a reviewer
:). Speaking as the author of that specific change, it only took a second to
type out the issue. I wouldn't mind making a habit of copy/pasting the
description if other reviewers would find that more convenient than clicking a
link to see it (although I wouldn't want to make that a policy, given that
larger and more nuanced descriptions should generally live where they live so
they don't need to be manually synced up).
> Here is an example of a PR that was merged without an associated GitHub Issue
> https://github.com/twisted/twisted/pull/12095
>
> Do you see any disadvantages of merging such a PR?
Yes, it is a violation of project policy, which means that it violates an
assumption that we are supposed to be trying to uphold on merges to trunk. It
adds a weird edge case to handle in historical data if we want to have scripts
which look at news files and PRs, for example. It also can't follow the revert
policy if there's a problem, because there is no issue to be reopened to track
the work getting re-integrated; we just need a new disconnected PR, which makes
it more annoying to track reopens and hotspots for repeat work.
If we can agree that this is an acceptable cost to reduce friction for this
sort of change, then we need to accommodate this assumption, so it's less of a
big deal to do this once the decision has been made. To change this policy,
for example, we would have to come up with what exemptions are supposed to look
like on the revert side as well, and then there wouldn't be the possibility for
ambiguity there.
I won't say that we should never have violations of policy around judgement
calls. Our current policy, for example, does not contemplate the possibility
of trunk test failures caused by external changes, and thus it is overly
conservative in the case where CI is already broken and all work is already
blocked. Given that the whole thing is written to avoid blocking work on
branches / PRs since latency to land things is sometimes slow, breaking policy
to get trunk working again (for example, if un-reviewed force-pushes to trunk
are required to debug a github actions failure without potentially days or
weeks of intervening latency) seems justifiable within the spirit of the policy
even if we should really have a discussion about that to adjust the letter to
match it. But a better way to handle procedural red tape like this which you
think is inconveniencing contributors is, as a reviewer, just do the part of
the process that they forgot; file the issue and push the correctly named
topfile to the branch. If this is sufficiently inconvenient to you that it's
preventing you from doing reviews then we should talk about that.
The contributor in this specific case (@alex) makes it a point to violate any
project policy steps he feels do not serve him personally, which is a bit
annoying, but on balance his changes are generally worthwhile enough to
accommodate this idiosyncrasy. Given his feelings about adhering to policy,
he's never going to sign up to be a regular reviewer, so nothing is really lost
by accommodating this in his case, or the case of others with similar track
records and opinions.
And I think this generalizes: as a reviewer, it's always reasonable to make a
judgement call about whether you're going to do the work on the contributor's
behalf. Sometimes familiarizing someone with a part of the process ("you
should really write tests") is well worth blocking the work in question in
developing a contributor's skills and ensuring quality. Other times ("you
forgot a misc news fragment") you can just note it and take a second to do the
work yourself.
> What do you think?
>
> Thanks for the feedback.
>
>
> --
> Adi Roiban
> _______________________________________________
> Twisted mailing list -- [email protected]
> To unsubscribe send an email to [email protected]
> https://mail.python.org/mailman3/lists/twisted.python.org/
> Message archived at
> https://mail.python.org/archives/list/[email protected]/message/L6GEKQYSWUX4NBIHYEML3AKB2PXDD56N/
> Code of Conduct: https://twisted.org/conduct
_______________________________________________
Twisted mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://mail.python.org/mailman3/lists/twisted.python.org/
Message archived at
https://mail.python.org/archives/list/[email protected]/message/U3HQEZM5RTUQZ3MLHBR5WVT2JXIPPQZY/
Code of Conduct: https://twisted.org/conduct
