Skip to content

Remove a category label needs-* when the user is defining one #370

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.

Sign up for GitHub

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
danilo-gemoli wants to merge 1 commit into from
Closed

Remove a category label needs-* when the user is defining one #370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions pkg/plugins/label/label.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,23 @@ func getLabelsFromGenericMatches(matches [][]string, labelFilter func(string) bo
return labels
}

// getCategoriesFromLabels extracts the categories from `labels` that are also in `needsLabels`.
// Example:
//
// labels := []string{"triage/unresolved", "area/testing"}
// needsLabels := []string{"triage"}
Copy link
Contributor

@petr-muller petr-muller Feb 4, 2025

Choose a reason for hiding this comment

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

nit : weird indent

// result := []string{"triage"}
func getCategoriesFromLabels(labels, needsLabels []string) sets.Set[string] {
Copy link
Contributor

@jmguzik jmguzik Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
func getCategoriesFromLabels(labels, needsLabels []string) sets.Set[string] {
func getCategoriesFromLabels(labels, needsLabels []string) sets.Set[string] {
needsLabelsSet := sets.New(needsLabels...)
categories := sets.New[string]()
for _, label := range labels {
if prefix, _, found := strings.Cut(label, "/"); found && needsLabelsSet.Has(prefix) {
categories.Insert(prefix)
}
}
return categories
}

Just for you to consider

Copy link
Contributor

@petr-muller petr-muller Feb 4, 2025

Choose a reason for hiding this comment

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

Weak -1 from me, I'm a fan of private-unless-proven-to-be-necessary-public 😓

Copy link
Contributor

@jmguzik jmguzik Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Its my bad. I meant internals of the function, my intention was not to propose to make it public. Let me correct that.

Copy link
Contributor

@jmguzik jmguzik Feb 4, 2025

Choose a reason for hiding this comment

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

Corrected, again comment is about internals not about publicity, sorry for the confusion.

needsLabelsSet := sets.New(needsLabels...)
categories := sets.New[string]()
for _, label := range labels {
if split := strings.Split(label, "/"); len(split) == 2 && needsLabelsSet.Has(split[0]) {
categories.Insert(split[0])
}
}
return categories
}

func handleComment(gc githubClient, log *logrus.Entry, config plugins.Label, e *github.GenericCommentEvent) error {
if e.Action != github.GenericCommentActionCreated {
return nil
Expand Down Expand Up @@ -201,13 +218,22 @@ func handleComment(gc githubClient, log *logrus.Entry, config plugins.Label, e *
// Get labels to add and labels to remove from regexp matches
labelsToAdd = append(getLabelsFromREMatches(labelMatches), getLabelsFromGenericMatches(customLabelMatches, labelFilter, &nonexistent)...)
labelsToRemove = append(getLabelsFromREMatches(removeLabelMatches), getLabelsFromGenericMatches(customRemoveLabelMatches, labelFilter, &nonexistent)...)
categoriesToAdd := getCategoriesFromLabels(labelsToAdd, needsLabels)
issueLabelsSet := sets.New(issueLabels...)

for _, needsCategory := range needsLabels {
needsLabel := fmt.Sprintf("needs-%s", needsCategory)
if !RepoLabelsExisting.Has(needsLabel) {
// Repo doesn't have the needs-* label.
continue
}

// Remove a category needs-* if any label is defining it. As an example:
// the issue has `needs-triage` and the user is commenting `/triage unresolved`
if issueLabelsSet.Has(needsLabel) && categoriesToAdd.Has(needsCategory) {
labelsToRemove = append(labelsToRemove, needsLabel)
}

removed := labelsWithCategory(labelsToRemove, needsCategory)
if removed.Len() == 0 || labelsWithCategory(labelsToAdd, needsCategory).Len() > 0 {
// If a category is not being removed, or also being added, don't add needs-* label.
Expand Down
32 changes: 32 additions & 0 deletions pkg/plugins/label/label_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,38 @@ func TestHandleComment(t *testing.T) {
action: github.GenericCommentActionCreated,
expectedRemovedLabels: formatWithPRInfo("restricted-label"),
},
{
name: "Remove triage category via /triage command",
body: "/triage needs-information",
repoLabels: []string{"area/infra", "triage/needs-information", "needs-triage"},
issueLabels: []string{"area/infra", "needs-triage"},
expectedNewLabels: formatWithPRInfo("triage/needs-information"),
expectedRemovedLabels: formatWithPRInfo("needs-triage"),
commenter: orgMember,
action: github.GenericCommentActionCreated,
},
{
name: "Remove triage category via /label command",
body: "/label triage/needs-information",
extraLabels: []string{"triage/needs-information"},
repoLabels: []string{"triage/needs-information", "needs-triage"},
issueLabels: []string{"needs-triage"},
expectedNewLabels: formatWithPRInfo("triage/needs-information"),
expectedRemovedLabels: formatWithPRInfo("needs-triage"),
commenter: orgMember,
action: github.GenericCommentActionCreated,
},
{
name: "Nonstandard needs-* label, do not remove",
body: "/label fake/whatever",
extraLabels: []string{"fake/whatever"},
repoLabels: []string{"needs-fake", "fake/whatever"},
issueLabels: []string{"needs-fake"},
expectedNewLabels: formatWithPRInfo("fake/whatever"),
expectedRemovedLabels: []string{},
commenter: orgMember,
action: github.GenericCommentActionCreated,
},
}

for _, tc := range testcases {
Expand Down