imsky / pull-review Goto Github PK

apparently the closing ] is ignored, should fix

also fix the error display

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

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

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

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

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:

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

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

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

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.

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

also add version

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 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

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

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

Error: no_text seems highly correlated with this

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;
      }
    }
  });

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

make it clear in docs that order matters

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

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

consider gini coefficient

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