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
Update: enforceForOrderingRelations no-unsafe-negation (fixes #12163) #12414
Conversation
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.
Thanks for the PR! I left some notes for tests and the documentation.
- typos - clarity of docs - extra test cases
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 good to me, once @mdjermanovic's concerns are addressed.
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.
Thanks! Just one small detail in the docs.
Could you please also change the PR title to end with (fixes #12163)
? Here are some examples.
To avoid confusion, there is no need to change commit messages. Since this PR has multiple commits, the convention from the link above applies to the PR title.
docs/rules/no-unsafe-negation.md
Outdated
|
||
### enforceForOrderingRelations | ||
|
||
With this option set to true, the rule is additionally enforced for: |
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.
It would be good to put backticks around true
Thanks @mdjermanovic. I've addressed the change and fixed the PR title. |
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.
LGTM, thanks!
@platinumazure would Update: enforceForOrderingRelations no-unsafe-negation (fixes #12163)
be a more descriptive title?
@mdjermanovic I'm not really worried in this case, because I'm pretty sure we would add more information about this change in the Highlights section of the release notes. So we can clarify the option name at that point. If you feel strongly, though, feel free to leave a Request Changes review (as I have no objection to the change either). |
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.
@samrae7 Could we please insert the option's name in the PR title, I believe it's better for the commit history.
It could be something like Update: enforceForOrderingRelations no-unsafe-negation (fixes #12163)
It isn't a correct sentence, but the keywords are included :-)
@mdjermanovic sure, done. |
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.
Thanks!
Merged! Thanks @samrae7 for contributing! |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Add
enforceForOrderingRelations
option to no-unsafe-negation rule. See:#12163
Is there anything you'd like reviewers to focus on?