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

ARROW-12653: [Archery] allow me to add a comment to crossbow requests #10247

Closed
wants to merge 5 commits into from

Conversation

jonkeane
Copy link
Member

@jonkeane jonkeane commented May 4, 2021

No description provided.

@github-actions
Copy link

github-actions bot commented May 4, 2021

@jonkeane
Copy link
Member Author

jonkeane commented May 4, 2021

@github-actions autotune

@@ -156,6 +156,7 @@ def handler(command, **kwargs):

@pytest.mark.parametrize(('command', 'reaction'), [
('@ursabot build', '+1'),
('@ursabot build\nwith a comment', '+1'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Jon for working on this!

Could you please add a test case with additional content preceding the mention?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely, I wasn't 100% certain this was the appropriate place to test this — so if it's not let me know.

Also, I expect that test to fail (and respond a -1), though we could probably add support for it if we wanted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that it is a no-op, just like other examples where the @ mention is not first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonkeane thanks for adding a test case!

We should probably add support for @ mention anywhere in the comment by splitting the lines first.
Could you please create a follow-up jira for this use case (or directly implement it :) in this PR)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made ARROW-12720 I started thinking about doing that splitting, but I'm not totally sure it's a good idea to allow the command to be anywhere (or anywhere at the beginning of a newline). The current code requires that the comment start with the mention which wasn't changed in this PR. At the very least we should discuss/decide what the boundaries ought to be there, which we can do on that ticket before changing that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@jonkeane jonkeane closed this in ebf0191 May 10, 2021
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
Closes apache#10247 from jonkeane/ARROW-12653

Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jonathan Keane <jkeane@gmail.com>
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

Successfully merging this pull request may close these issues.

None yet

2 participants