-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add allowLabels option to no-missing-label-refs #513
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for working on it. Just one thought: Would anyone be open to renaming the option to Since most rules in markdown use markdown/src/rules/no-duplicate-definitions.js Lines 70 to 71 in c790317
markdown/src/rules/no-empty-definitions.js Lines 89 to 90 in c790317
Line 70 in c790317
markdown/src/rules/no-unused-definitions.js Lines 71 to 72 in c790317
|
Yeah, that makes sense 👍. I went ahead and updated the option name. |
|
||
The following options are available on this rule: | ||
|
||
* `allowLabels: Array<string>` - labels to allow when checking for missing label references. (default: `[]`) |
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.
In your original issue you mentioned wanting to use ignoreLabels
. Why the change?
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.
I initially called it ignoreLabels
, but @lumirlumir suggested being consistent with other rules, so I renamed it. Would you like me to revert the change?
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.
I've asked @jaymarvelz (and other team members) for their opinion on #513 (comment), how about renaming it to allow
for consistency across the Markdown rules.
If you feel ignore
is more appropriate here, please disregard my suggestion and feel free to keep ignore
.
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.
allowLabels
amkes sense to me 👍🏻
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.
Looks so good to me. Thanks 👍
Would like @nzakas to verify (including the option name) before merging.
Prerequisites checklist
What is the purpose of this pull request?
This PR adds a new
allowLabels
option tono-missing-label-refs
, allowing users to intentionally permit specific labels without definitions.What changes did you make? (Give an overview)
allowLabels
.Related Issues
Fixes #449
Is there anything you'd like reviewers to focus on?