-
Notifications
You must be signed in to change notification settings - Fork 119
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
Auto label node PR's #6
Conversation
I don't mind if it over-labels a bit. We can probably do a trial of sorts by just regexing against tag names with some sort of priority, falling back to |
the refactoring bits lgtm |
be80345
to
a24da27
Compare
Pushed some improvements. Now it does some very basic analysis on the file paths changed and returns a list of labels. Obvious things still missing:
|
I think it is probably more reliable to just hardcode a bunch of regex's. I'm willing to figure out what those should be. We should also try to fallback to |
I need the github client extracted out as you did here in For now I'll use a copy of that file but won't commit it. |
@phillipj How about we make this only check against regex's and only use This will also make sure the bot never uses labels that people willy-nilly add/remove. |
@Fishrock123 sure, we could start off with a limited set of labels. What about keeping the current |
@phillipj Also seems good to me, but I think it should be cleaned up a bit to be a bit more portable.. We probably need some to check Another example would be a purely benchmarks change. I don't think we should add that yet, just let it be easier to build on after, if possible. :) |
60e24d2
to
6fdd91f
Compare
|
||
// | ||
// Planned tests to be resolved later | ||
// |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@phillipj lgtm minus a nit, although I'm not sure we should deploy this yet. We'll want to have a go-ahead from collaborators with some example in a test repo I think. If you think we should go straight to deploy though, I'm willing to back that. |
6fdd91f
to
ff1b338
Compare
ff1b338
to
dfd437f
Compare
@Fishrock123 PTAL This branch is currently deployed and have done some initial tests in https://github.com/TestOrgPleaseIgnore/node/pulls |
user: options.owner, | ||
repo: options.repo, | ||
number: options.prId, | ||
labels: labels |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Started out with two labels in this iteration: c++ and test.
dfd437f
to
2b099db
Compare
@Fishrock123 @williamkapke Thanks for reviewing! Just pushed an update which merges the resolved labels with the PR's existing labels, so that we don't accidentally remove any labels. If it looks good to you, merge at will. |
Forgot to mention I tested it with three PRs (9 - 11) in the test repo https://github.com/TestOrgPleaseIgnore/node/pulls |
Seems good to me, let's try it imo. |
Very much a work in progress ATM, just wanted to get this out there for public scrutiny and work on it the next couple of days.
Goals
/labels/#PR-NUM
UPDATE In the discussion below, we've decided to only resolve
c++
andtest
labels at first, then add more labels in later iterations of this.Refs #1