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

2020-05-17 Thread timothee
> > 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 


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

2020-05-17 Thread miran
> 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.


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

2020-05-17 Thread dom96
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.


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

2020-05-17 Thread timothee
Currently, looking at 
[https://github.com/nim-lang/Nim/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc](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://su2code.github.io/docs/Code-Review)/
  * 
[https://medium.engineering/code-reviews-at-medium-bed2c0dce13a](https://medium.engineering/code-reviews-at-medium-bed2c0dce13a)



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