Skip to content

Fix case label removal#2368

Open
JojoFlex1 wants to merge 4 commits intorust-lang:masterfrom
JojoFlex1:fix-case-insensitive-label-removal
Open

Fix case label removal#2368
JojoFlex1 wants to merge 4 commits intorust-lang:masterfrom
JojoFlex1:fix-case-insensitive-label-removal

Conversation

@JojoFlex1
Copy link
Copy Markdown

So like when removing labels using @rustbot label -E-needs-mcve, the removal fails for labels with capital letters. This is cause the comparison in remove_labels was case-sensitive so -e-needs-mcve wouldn't match E-needs-mcve on the issue.
Adding labels works fine because GitHub's API handles case-insensitivity on their end. But removing requires an exact match with the label name as it exists on GitHub.
Now
Changed filter and contains to filter_map + find with a case-insensitive comparison. This also ensures we use the actual label name from the issue in the DELETE request, not the user-typed version.
#2214

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 11, 2026

Error: Label E-needs-mcve can only be set by Rust team members

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

@apiraino
Copy link
Copy Markdown
Contributor

@JojoFlex1 did you (or could you) test this change?

Self-assigning for review.

thanks

@apiraino apiraino self-assigned this Apr 11, 2026
@JojoFlex1
Copy link
Copy Markdown
Author

@apiraino at first i ran cargo build and cargo test relabel and all 7 tests passed. So now i added a unit test specifically for the case-insensitive matching in src/github/issue.rs.

Comment thread src/github/issue.rs Outdated
.find(|existing| existing.name.to_lowercase() == l.name.to_lowercase())
.cloned()
})
.collect();
Copy link
Copy Markdown
Member

@marcoieni marcoieni Apr 13, 2026

Choose a reason for hiding this comment

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

View changes since the review

this isn't a unit test because you aren't testing you code. I.e. you aren't calling a function of your code.

@JojoFlex1
Copy link
Copy Markdown
Author

@marcoieni I extracted the filtering logic into a helper function find_label_case_insensitive and updated the test to call it directly.
I didn't do a live end-to-end test since remove_labels makes real GitHub API calls which makes it harder to test directly the helper function approach lets us test the core logic . If there's a better way to test this or if you have any other suggestions, Im open to learn.

@apiraino
Copy link
Copy Markdown
Contributor

@marcoieni I extracted the filtering logic into a helper function find_label_case_insensitive and updated the test to call it directly. I didn't do a live end-to-end test since remove_labels makes real GitHub API calls which makes it harder to test directly the helper function approach lets us test the core logic . If there's a better way to test this or if you have any other suggestions, Im open to learn.

@JojoFlex1 End-to-end testing is detailed in the README. If you find yourself not being able to do that, I'll get around to it (but not immediately).

@JojoFlex1
Copy link
Copy Markdown
Author

@apiraino okay sure

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.

4 participants