On Jan 29, 2024, at 10:36 AM, Adi Roiban <adiroiban@gmail.com> 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-h...
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.BufferingTLSTra...
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 -- twisted@python.org To unsubscribe send an email to twisted-leave@python.org https://mail.python.org/mailman3/lists/twisted.python.org/ Message archived at https://mail.python.org/archives/list/twisted@python.org/message/L6GEKQYSWUX... Code of Conduct: https://twisted.org/conduct