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
fix(require-tothrow-message): rename rule to require-to-throw-message #306
fix(require-tothrow-message): rename rule to require-to-throw-message #306
Conversation
672917b
to
b42638f
Compare
Seems like a reasonably gentle way to do it if we decide to do the rename. For the test, just increasing the count by 1 until it's removed in a major is probably fine, the docs test might need a whitelist. |
37a5a1f
to
a6467a6
Compare
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! Left some inline comments
Just landed the TS rewrite of this rule, mind rebasing? Absolutely no rush as we're still some way off a new major, but I thought I'd flag it early 🙂 |
84138e2
to
b2a7f7d
Compare
I wasn't sure how to properly rebase so I manually added the changes and force pushed. Here are the commits before the rebase. @SimenB This PR does not have any breaking changes because it deprecates the old |
) { | ||
// Look for `toThrow` calls with no arguments. | ||
context.report({ | ||
messageId: 'requireRethrow', // todo: rename to 'addErrorMessage' |
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 wouldn't object if you wanted to do these todo
s for me as part of this PR 😉
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.
Would that be a breaking change? I am not too familiar how messageId
is used.
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.
No, it's not breaking - messageId
is just used internally by eslint
to make it easier to test, so you don't have to write things out multiple times.
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 updated this PR to include the changes in TODO. Please verify that the context.report.data.propertyName
-> context.report.data.matcherName
was wanted.
src/__tests__/rules.test.ts
Outdated
'require-tothrow-message', | ||
]; | ||
|
||
const ruleNames = Object.keys(plugin.rules).filter(rule => { |
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.
This could be short-handed
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 generally don't use implicit returns on arrow functions because I think they are harder to work with, but it doesn't matter to me either way. Do you want me to change it?
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 think I would, if you don't mind; we use shorthand arrow functions in most other places, and imo they can make filters easier to read by providing a clearer distinction between simple and advance filters.
For example, in this case the filter can just be filter(rule => !excludeRules.includes(rules));
, which reads as "filter rules not included in excludeRules".
Any conditional more complex than this and I'd agree w/ you that it should be long form.
They tend to be nicer w/ TypeScript anyway, since you don't have to worry about returns being used by mistake if you short-hand.
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 - left a few actionable comments, but nothing blocking.
b2a7f7d
to
51282f0
Compare
I've resolved the conflicts. Let me know how I should proceed with the remaining PR comments. |
src/__tests__/rules.test.ts
Outdated
'require-tothrow-message', | ||
]; | ||
|
||
const ruleNames = Object.keys(plugin.rules).filter(rule => { |
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 think I would, if you don't mind; we use shorthand arrow functions in most other places, and imo they can make filters easier to read by providing a clearer distinction between simple and advance filters.
For example, in this case the filter can just be filter(rule => !excludeRules.includes(rules));
, which reads as "filter rules not included in excludeRules".
Any conditional more complex than this and I'd agree w/ you that it should be long form.
They tend to be nicer w/ TypeScript anyway, since you don't have to worry about returns being used by mistake if you short-hand.
@chrisblossom Sorry for the delay in getting back to you. I'm happy w/ this aside from the requested change. Cheers for actioning the Once you've made the requested change, I'll merge this, and then someone will do a release. |
Thanks @G-Rath for the response! I think I've made the final requested change. If there is anything else please let me know. |
@chrisblossom sorry I forgot about this 😬 I'm prepping a |
resolved the conflict, made breaking, and changed to target |
This change makes consistent with other rule naming format and removes spelling error message from editors.
This is not ready to be merged. Just more of a proof of concept / there is probably a better way within eslint to rename rules. Also
src/__tests__/rules.test.js
currently fails tests.Closes #296.