Skip to content

Conversation

mibac138
Copy link
Contributor

@mibac138 mibac138 commented Jun 7, 2021

Fixes #714

I believe the proposed include_labels is already added as required_labels, correct me if I'm wrong though.

@nikomatsakis
Copy link
Contributor

This looks good to me. The only question would be whether it should be "any" (i.e., disallowed it any of the labels match, versus all). But that seems to match the existing option.

@mibac138
Copy link
Contributor Author

mibac138 commented Jun 8, 2021

I'm not sure what exactly prompted issue #714 but I feel like this version should be enough for simple cases, and like you said, it matches the existing option. If need be, it can always be expanded

@Mark-Simulacrum
Copy link
Member

Once conflicts are resolved here I'd be happy to merge.

@Urgau
Copy link
Member

Urgau commented Jun 17, 2025

@mibac138, still interesting in moving this change forward?

Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

Impl looks good, but this PR needs to be rebased. Once that's done we can merge it.

View changes since this review

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.

Add {include|exclude}_labels options to notify-zulip
4 participants