bors argument parsing does not report errors

rust-lang / homu

A bot that integrates with GitHub and your favorite continuous integration service

MIT License

182 stars 57 forks source link

bors argument parsing does not report errors #8

Open RalfJung opened 6 years ago

RalfJung commented 6 years ago

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 commented 6 years ago

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

kennytm commented 6 years ago

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

bryanburgers commented 5 years ago

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.

Centril commented 5 years ago

and treeclosed+.

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

kennytm commented 5 years ago

It would mean treeclosed=1

Centril commented 5 years ago

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

RalfJung commented 5 years ago

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?

Centril commented 5 years ago

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

RalfJung commented 5 years ago

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

Centril commented 5 years ago

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