-
Notifications
You must be signed in to change notification settings - Fork 15
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
Teach scc merge
to parse comments for labels
#125
Conversation
With this commit, if any line starts with "--label ", the following content will be treated as label and used for exclusion/inclusion.
@@ -49,6 +49,14 @@ def tearDown(self): | |||
self.sandbox.push_branch(":%s" % self.branch, remote=self.user) | |||
super(TestMerge, self).tearDown() | |||
|
|||
def create_status(self, state): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the move of this block? To keep it alphabetical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Primarily to separate setup/helper methods from proper test methods. But it doesn't matter that much so I can restore the old behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need. Just wondering if there was a new policy.
How is it intended to remove labels via |
What do you mean by remove labels? Unlink label from the issue? |
Well, if one comment allows: |
After discussion with @joshmoore, the next steps for this PR are:
All the dynamic action labelling ( |
- run_filter() now returns a reason together with the filter status - unit tests have been fixed accordingly - status-excluded PRs are now included as part of the excluded PRs section
NB: with the three last commits, the output of
|
Add unit test with a whitelisting function checking organization members
- Check for exclude comments ahead of PR filters and fail-fast if found - Mark comment-excluded PRs separately - Reuse the default filter mode for whitelisting PR comments - Fix documentation of default mode in the command help
With the last two comments, whitelisting should be turned on for parsing
|
Travis is green and changes look good. Only other verification we wanted was SCC-self-merge, correct? |
Yes, especially the 2 first tests of http://hudson.openmicroscopy.org.uk/view/Mgmt/job/SCC-self-merge/180/testReport/test.integration.TestMerge/TestMerge/. |
👍 |
Teach `scc merge` to parse comments for labels
All merge commands, e.g.
scc merge
should be able to parse--exclude
in the comments/description. Such comments will exclude the PR during the merge operation.