Skip to content

Conversation

@fivecar
Copy link

@fivecar fivecar commented Feb 14, 2025

This PR:

  • Accepts mixed-case owner names
  • Adds a test to that effect
  • Updates the README to clarify the instructions and correctly state that blacklisted_labels is required.
  • Changes the error message to read more understandably (i.e. the original text had inverted logic in its text).

This PR:
- Accepts mixed-case owner names
- Adds a test to that effect
- Updates the README to clarify the instructions and correctly state
  that `blacklisted_labels` is required.
- Changes the error message to read more understandably (i.e. the
  original text had inverted logic in its text).
@fivecar
Copy link
Author

fivecar commented Feb 14, 2025

@dkhmelenko : hi there! Thanks for this super-helpful tool. Would you mind reviewing this? I had run into this problem in my own use of the tool.


### blacklisted_labels

**mandatory**
Copy link
Owner

Choose a reason for hiding this comment

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

why do you make blacklisted_labels as a mandatory property?

Copy link
Author

Choose a reason for hiding this comment

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

@dkhmelenko : please correct me if I'm wrong, but I ran into #753 (i.e. my GitHub Action fails if my autoapprove.yml doesn't have the blacklisted_labels field, and succeeds if it does [with an empty array]).

Comment on lines +43 to +44
const lowerLogin = pr.user.login.toLowerCase()
const ownerSatisfied = config.from_owner.length === 0 || config.from_owner.some((owner: string) => owner.toLowerCase() === lowerLogin)
Copy link
Owner

Choose a reason for hiding this comment

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

That's an interesting case, out of curiosity: when can it happen that we have different cases of the owner? Or do you want to prevent a situation when GitHub username has different case than the one in the config file?

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to address the situation where the yml file contains a different case from what GitHub has. Since GitHub handles are case-insensitive, it feels like the yml format should similarly not enforce casing.

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.

2 participants