homu icon indicating copy to clipboard operation
homu copied to clipboard

bors argument parsing does not report errors

Open RalfJung opened this issue 7 years ago • 11 comments

I recently issued the command @bors r+ rollup+, because I have seen plenty of cases where one has to add a + after a command to make it happen. Not so for rollup, it seems, but bors also did not complain. Can we get error messages when there is a word+ or word- but bors does not actually recognize this as a command?

RalfJung avatar Sep 01 '18 10:09 RalfJung

Oh, and as a bonus (but less important), accepting rollup+ as well as rollup would be nice :)

RalfJung avatar Sep 01 '18 10:09 RalfJung

I think it's better just let homu recognize rollup+, try+ and treeclosed+.

kennytm avatar Sep 01 '18 12:09 kennytm

This is a good first issue for somebody new to Homu.

A little guidance for somebody taking this task:

  • [ ] Create a test similar to https://github.com/rust-lang/homu/blob/abd0083ba6a738e0bc1a1bc69e64676ff9032ebf/homu/tests/test_parse_issue_comment.py#L257-L269 that sets the body variable to a comment you might expect – one that includes a @bors retry+ command.
  • [ ] Observe that the test fails when you run python3 setup.py test.
  • [ ] Update https://github.com/rust-lang/homu/blob/abd0083ba6a738e0bc1a1bc69e64676ff9032ebf/homu/parse_issue_comment.py#L139 to pass the test.
  • [ ] Identity other similar commands that would benefit from the {commandname}+ syntax and make the same changes for those commands.

bryanburgers avatar Jul 01 '19 15:07 bryanburgers

and treeclosed+.

@kennytm What does this even mean if there's no priority?

Centril avatar Jul 01 '19 16:07 Centril

It would mean treeclosed=1

kennytm avatar Jul 01 '19 17:07 kennytm

@kennytm I would always like that to scream loudly that I've done something wrong because >= 1 is entirely ineffective.

Centril avatar Jul 01 '19 17:07 Centril

I have no mental model as to why some things have a + suffix and some don't.

I mean, treeclose- is a thing, right? So then not having treeclose+ seems really weird?

RalfJung avatar Jul 01 '19 17:07 RalfJung

@RalfJung treeclosed- means that you remove the previous treeclosed=P. If you use rollup- it means removing the rollup annotation; but treeclosed+ meaning treeclosed=1 is "a bug" from a UX perspective of the person doing the treeclosing. If treeclosed+ was equivalent to treeclosed=500 I would be fine with it.

Centril avatar Jul 01 '19 17:07 Centril

So treeclosed=0 is not "un-closing" the tree? That is surprising, too.

RalfJung avatar Jul 01 '19 18:07 RalfJung

I don't know; I've never tried it :)

Centril avatar Jul 01 '19 18:07 Centril