Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Autolabel new Pull Requests #29

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

Closed
wants to merge 4 commits into from

Conversation

javiereguiluz
Copy link
Contributor

Two important things:

  1. I haven't tested this in any way and I don't know how to test it in a real repository.

  2. What happens if you try to label an issue with a non-existent label? I guess it will be dropped silently. Otherwise, we'll have a problem if a user creates a PR with a title like this: "[Whatever] Blah blah" FIXED in the latest version of this PR.

*
* @return string The new status
*/
public function handlePullRequestCreatedEvent($prNumber)
public function handlePullRequestCreatedEvent($prNumber, $prTitle, $prBody)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we listen to this event, we should also listen for changes to the title/body after the PR was created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably ... but we can leave that enhancement for the future.

@javiereguiluz
Copy link
Contributor Author

If this feature is finally approved, should we ...

  • ... merge it as soon as possible to confirm is ready for the Hack Day of this Saturday?
  • ... or merge it after this Saturday to not interfere with the Hack Day?

Thanks!

@javiereguiluz
Copy link
Contributor Author

Friendly ping to not forget about this PR. What could we do to move it forward? Thanks!

@sstok
Copy link
Contributor

sstok commented Mar 22, 2016

@javiereguiluz Tests are missing, overall it looks pretty good 👍

@javiereguiluz
Copy link
Contributor Author

@sstok thanks for the review. I've fixed the failing test.

@weaverryan
Copy link
Contributor

See #34 instead. @javiereguiluz we made such big changes to the repo, it wasn't fair to have you try to figure out the conflict :)

@weaverryan weaverryan closed this May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.