Mirror of forum.nim-lang.org

6348 :: proposal: PTAL tag to make it clear that a PR is ready again for review

• timothee (orginal) [2020-05-17T09:14:20+02:00] view original

  Currently, looking at https://github.com/nim-lang/Nim/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc it's not clear which PR's are ready for review vs still being worked on:

  • using draft vs not draft is not good for that: github recently allowed marking a non-draft PR as draft, but that feature is unavailable if you're not a project member; also a PR that merely needs to address comments shouldn't really be considered a draft anyways (it works but just needs to address comments)
  • using CI passing vs failing is not good either because of flaky tests, and because a PR could have pending unaddressed comments and still be green

  proposal

  • add a new "PTAL" github tag, with the meaning: either PR is ready for review for the 1st time, or, after a round of review and after all comments were addressed, it's ready again for another round; and all failures are (to PR author's knownledge) unrelated to the PR
  • users who don't have permissions to add a tag to their own PR can add [PTAL] to their PR title

  the feature would be optional to use, but PR authors who want to use it can, to increase visibility of their PR (once they've addressed all comments) without having to ping reviewers

  it makes those PR's easy to search for and we can easily filter by those in github search UI

  note

  PTAL simply means "Please Take Another Look", this practice is common both in tech companies as well as in open source, eg:

  • https://su2code.github.io/docs/Code-Review/
  • https://medium.engineering/code-reviews-at-medium-bed2c0dce13a

  and unfortunately github doesn't have this feature builtin, so a tag is the best workaround

---

• dom96 (orginal) [2020-05-17T13:03:32+02:00] view original

  Please no. If you want someone to have a look at your PR then bump the PR so it gets put at the top of our notifications or just ping the best person to review that PR (if you know who they are). All these title/tag changes are just noise and we've got enough of that already.

---

• miran (orginal) [2020-05-17T17:09:21+02:00] view original

  Please no.

  +1

  All these title/tag changes are just noise and we've got enough of that already.

  +1

  just ping the best person to review that PR

  Or, better yet, don't ping anybody and just be patient.

  At the time of writing this, we have 114 open PRs (and that's after two weeks of constant merging as fast as possible! The number was much higher last month.), and it should be obvious to anybody following Nim's github repo that we have very hard time reviewing and merging them in reasonable time.

  One person pinging and/or PTAL-ing (which already happens), just gives an impression "look at my stuff, my stuff is more important than all the other stuff by all the other contributors". NO!

  @timothee If you really want to help us with faster merging of PRs, take a look at those 40 out of 114 (35%!) open PRs which are authored by you. Are all of them really important (for larger Nim audience, not just for your workflow)? How can we review PRs in time if you're constantly flooding us with new stuff?

  Long story short: new flags or constant pings don't help with faster reviews. Less "privatization" of Nim repo will.

---

• timothee (orginal) [2020-05-18T02:52:27+02:00] view original

  > look at my stuff, my stuff is more important than all the other stuff by all the other contributors

  that's not the point at all, quite the opposite.

  The whole point is for triaging PRs and saving reviewer time by allowing them to filter from those 114 open PR's the ones that are in a ready for review state, without needing to ping anyone / polling

  so if someone has N open PRs but only 4 of them are ready for 1st/next round of review, meaning:

  • all comments addressed
  • no git conflicts
  • all tests pass except for unrelated failures

  then the filtering will only show 4 for that user

---