Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove support for bors commands in review comments #41

Closed
bryanburgers opened this issue May 29, 2019 · 2 comments
Closed

Remove support for bors commands in review comments #41

bryanburgers opened this issue May 29, 2019 · 2 comments

Comments

@bryanburgers
Copy link
Contributor

There are 3 types of comments that can be made related to a pull request:

  1. Issue comments, which – despite their name – are comments made directly in the pull request. Most of our @bors commands are made here.
  2. Review comments, which are comments made on the code diff of a merge request
  3. Commit comments, which are comments made on the commit itself (and we do not support in @bors).

I wrote a script to go through all of our pull requests to quantify how many bors commands appear in review comments. Here are the results:

Repo Review comment commands
rust-lang/cargo 0
rust-lang/rust-clippy 0
rust-lang-nursery/compiler-builtins 0
rust-lang-nursery/crater 0
rust-lang/hashbrown 0
rust-lang/libc 0
rust-lang/regex 0
rust-lang/rls 0
rust-lang/rust 1
rust-lang/rustlings 0
rust-lang/rustup.rs 0

So, the only time this feature has been used (that I can find; I won't rule out that my code was not 100% perfect) was this 5 Dec 2018 comment.

In the spirit that the easiest code to maintain is no code, I propose that we remove support for this rarely used feature. The total cost of supporting it is pretty low, but the utility of this function appears even lower (one use in 5 years).

I believe it would mostly come down to removing the following two parts of the code:

homu/homu/main.py

Lines 1507 to 1522 in df43bc0

for comment in pull.iter_comments():
if comment.original_commit_id == pull.head.sha:
parse_commands(
comment.body,
comment.user.login,
repo_label,
repo_cfg,
state,
my_username,
db,
states,
sha=comment.original_commit_id,
command_src=comment.to_json()['html_url'],
# FIXME switch to `comment.html_url`
# after updating github3 to 1.3.0+
)

homu/homu/server.py

Lines 372 to 402 in df43bc0

if event_type == 'pull_request_review_comment':
action = info['action']
original_commit_id = info['comment']['original_commit_id']
head_sha = info['pull_request']['head']['sha']
if action == 'created' and original_commit_id == head_sha:
pull_num = info['pull_request']['number']
body = info['comment']['body']
username = info['sender']['login']
state = g.states[repo_label].get(pull_num)
if state:
state.title = info['pull_request']['title']
state.body = info['pull_request']['body']
if parse_commands(
body,
username,
repo_label,
repo_cfg,
state,
g.my_username,
g.db,
g.states,
realtime=True,
sha=original_commit_id,
command_src=info['comment']['html_url'],
):
state.save()
g.queue_handler()

@Mark-Simulacrum
Copy link
Member

I'm somewhat ambivalent on this. It's certainly a simplification, but it also seems somewhat odd to not support review comments as from a GH UI perspective they're quite similar. I honestly didn't think we supported them and seem to recall instances where I didn't use them as a result, so I think the statistics may be flawed in that regard.

@bryanburgers
Copy link
Contributor Author

As I've thought about this more, it seems over-aggressive to prune a feature of pretty minimal cost. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants