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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

accept bors commands with a colon #232

Merged
merged 3 commits into from Jul 19, 2017

Conversation

Projects
None yet
3 participants
@khodzha
Copy link
Contributor

khodzha commented Jul 19, 2017

pr for issue #219
not sure if _parse is appropriate name for a function tho 馃

Edit by @notriddle: fixes #219

@Macarse

This comment has been minimized.

Copy link
Contributor

Macarse commented Jul 19, 2017

We are already reporting back when the command is not parsed correctly. I think this should be the same. Instead of accepting bors: +r bors should post saying: "Did you mean ..."

see #214

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

If you want suggestions, you could name the new function parse_cmd and rename the current function with that name to parse_trimmed_cmd.

I'm fine with merging it with just _parse, though; we can always rename it later.

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

@Macarse

Unlike the case of actual misspelled commands, I'm not sure what the advantage of rejecting bors: r+ is. It's obvious what the user means when they type that.

This PR only adds the : as a permitted delimiter between "bors" and the command. It doesn't make it accept +r as the review command.

@Macarse

This comment has been minimized.

Copy link
Contributor

Macarse commented Jul 19, 2017

@notriddle because you are extending the API. If bors: r+ is accepted, I also want bors; r+.

Why do we want to add this? To make sure developers do not waste time waiting for a build that never started?
Considering this a misspelled command would notify the developer immediately.

@khodzha

This comment has been minimized.

Copy link
Contributor Author

khodzha commented Jul 19, 2017

im not actually a bors-ng user, yet bors: r+ seems meaningful, bors; r+ does not
but i agree that bloating allowed separators can be a problem

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

Why do we want to add this? To make sure developers do not waste time waiting for a build that never started?

Mostly because it's a common thing I see homu users doing. I've already had to teach people who're migrating not to start the bot's name with an @ sign and that the command has to go on its own line. The "own line" thing is a restriction that I'm still not sure if I'm comfortable with, but at least there I can name an actual tradeoff in its favor (it reduces false positives like when people talk about bors's commands).

Carrying the colon thing into perpetuity seems like such a tiny burden compared to all the other compatibility stuff that bors-ng does. And it reduces friction.

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

@khodzha And thanks for putting up with our last-minute bickering. Honestly, it's kind of embarrassing that we haven't already resolved this 馃槥

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

To summarize: I filed issue #219 because it seemed like a good idea to make bors-ng's syntax more like homu's. To reduce friction for people migrating to it.

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

@khodzha ... And thank you for putting up with the 80char line limit. 馃榿

@Macarse

This comment has been minimized.

Copy link
Contributor

Macarse commented Jul 19, 2017

because it seemed like a good idea to make bors-ng's syntax more like homu's. To reduce friction for people migrating to it.

Posting a msg in the PR with the "Did you mean bors r+" also reduces the friction and teaches the user the correct and unique way of executing actions.

@notriddle As mentioned in the previous PR, this is your call. You should be the one making the final decision on the API we support.

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

Sure, okay.

bors r+

Next time, though, I'm opening a B-RFC issue before putting API-affecting changes on E-easy. Dropping an API design discussion on a first-time contributor is not the kind of environment that I think anyone wants to create for first-timers. If a contribution is well-made and does what the entry-level issue says to go through, nobody wants to end up being anxious about whether their thing will actually get merged.

bors bot added a commit that referenced this pull request Jul 19, 2017

Merge #232
232: accept bors commands with a colon r=notriddle

pr for issue #219
not sure if _parse is appropriate name for a function tho 馃 

*Edit by @notriddle: fixes #219*
@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

And thanks for showing up and (essentially) beta-testing the decision-making process, @khodzha.

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Jul 19, 2017

@bors bors bot merged commit f3b1635 into bors-ng:master Jul 19, 2017

2 checks passed

bors Build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@khodzha

This comment has been minimized.

Copy link
Contributor Author

khodzha commented Jul 19, 2017

you are welcome! 馃槂

@khodzha khodzha deleted the khodzha:bors_colon branch Jul 19, 2017

@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

Welp, that's done. It should be getting deployed to the test instance in a few minutes, and then it'll be ready for everyone to use.

notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Jul 19, 2017

Add bors-ng/bors-ng#232
Also add @khodzha to the "New Contributors" list.
@notriddle

This comment has been minimized.

Copy link
Member

notriddle commented Jul 19, 2017

@Macarse

This comment has been minimized.

Copy link
Contributor

Macarse commented Jul 19, 2017

@notriddle 100% agree.
@khodzha sorry for the discussion in your first PR! Second will go faster for sure 馃憤

@khodzha

This comment has been minimized.

Copy link
Contributor Author

khodzha commented Jul 19, 2017

no need for excuses, it was important thing to clear up 馃憤

notriddle added a commit to bors-ng/bors-ng.github.io that referenced this pull request Jul 29, 2017

Add bors-ng/bors-ng#232
Also add @khodzha to the "New Contributors" list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment