imsky / pull-review Goto Github PK

TODO: consider delegating to OWNERS

if there are multiple rules with the same prefix:

backend: alice, bob
backend/foo: charlie, bob
backend/foo/bar: charlie

and the file name starts with backend/foo/bar, alice/bob will get matched first, instead of charlie

should be clear it unassigns current assignees and ignores them for reassignment

make it clear in docs that order matters

in case Alice has changed more files than Bob, but has 1 files with almost zero ownership, Alice will be ranked lower than Bob, even if Bob has higher ownership in just one file

this prevents long files from dominating a pull request's blame metrics, e.g.

here Bob equal to Alice on LOC changed, even though Alice has much higher average ownership

sometimes min author diversity will assign a fallback reviewer that doesn't have enough context on a piece of code - in the worst case, just one reviewer. to mitigate against that, the original reviewer shouldn't be replaced, but retained alongside the new reviewer.

dry run currently outputs the review plan in human-readable form, but could also output it in a machine readable form

good to have "foo, bar please review this pull request #hubot-review" since the string then identifies every PR that hubot-review has seen

consider gini coefficient

Slack mentions no longer work for new users given https://api.slack.com/changelog/2017-09-the-one-about-usernames

possible alternatives: matching users by email, or matching users by name

Error: no_text seems highly correlated with this

also fix the error display

key image is not being set with this PR body:

![screen shot 2017-03-02 at 4 58 15 pm](https://cloud.githubusercontent.com/assets/123456/23528971/b1a5fc50-ff69-11e6-81d8-3e78836c215a.png)
![screen shot 2017-03-02 at 4 58 30 pm](https://cloud.githubusercontent.com/assets/123456/23528972/b1b50132-ff69-11e6-96ac-ed09bb4e8083.png)

for example, web/** files should not be allowed to be reviewed outside of specific Slack rooms

the issue is that the mapping function looks at only one of the real name or the username

  Object.keys(rawUsers).forEach(function(userId) {
    var user = rawUsers[userId];

    if (user.real_name) {
      if (chatUserMap[user.real_name]) {
        robot.logger.error('User mapping already exists for ' + user.real_name);
      } else {
        chatUserMap[user.real_name] = userId;
      }
    }

    if (user.name) {
      if (chatUserMap[user.name]) {
        robot.logger.error('User mapping already exists for ' + user.name);
      } else {
        chatUserMap[user.name] = userId;
      }
    }
  });

the blame-derived reviewers will get re-assigned and re-pinged, while the random/fallback reviewers may change

apparently the closing ] is ignored, should fix

also add version

not sure if min or max reviewers is ignored, but it's possible to "re-roll" review with someone already assigned

this should instead probably just blacklist the current assignees and let the planner come up with another set of valid reviewers, otherwise totally arbitrary assignments will be made

1. public unfurling may be undesirable when Slack itself does it
2. private unfurling may be undesirable as a user preference
3. unfurling the same link every time can be annoying (especially with images), should add a configurable cool-off period, e.g. 30s

this would listen for pull request comments like "/review" and run pull-review against the PR

also could avoid sending the "joint" message in the originating room

for example:

hello [world] [some link here](#)

this will "find" a Slack link: hello <#|world>