Skip to content
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

Merged

Conversation

chrisblossom
Copy link
Contributor

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.

@chrisblossom chrisblossom force-pushed the rename-require-tothrow-message branch from 672917b to b42638f Compare July 17, 2019 19:33
@jeysal
Copy link
Member

jeysal commented Jul 17, 2019

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.

@jeysal jeysal requested a review from SimenB July 17, 2019 21:44
@chrisblossom chrisblossom force-pushed the rename-require-tothrow-message branch 2 times, most recently from 37a5a1f to a6467a6 Compare July 18, 2019 20:48
src/__tests__/rules.test.js Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a 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

@SimenB
Copy link
Member

SimenB commented Aug 15, 2019

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 🙂

@chrisblossom chrisblossom force-pushed the rename-require-tothrow-message branch 2 times, most recently from 84138e2 to b2a7f7d Compare August 21, 2019 18:10
@chrisblossom
Copy link
Contributor Author

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 rename-require-tothrow-message rule instead of removing it, so if accepted, this should not need to wait for a major version. When there is a major released, we can remove the deprecated rule completely.

) {
// Look for `toThrow` calls with no arguments.
context.report({
messageId: 'requireRethrow', // todo: rename to 'addErrorMessage'
Copy link
Collaborator

@G-Rath G-Rath Aug 21, 2019

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 todos for me as part of this PR 😉

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

'require-tothrow-message',
];

const ruleNames = Object.keys(plugin.rules).filter(rule => {
Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

README.md Show resolved Hide resolved
Copy link
Collaborator

@G-Rath G-Rath left a 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.

@chrisblossom
Copy link
Contributor Author

I've resolved the conflicts. Let me know how I should proceed with the remaining PR comments.

'require-tothrow-message',
];

const ruleNames = Object.keys(plugin.rules).filter(rule => {
Copy link
Collaborator

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.

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 5, 2019

@chrisblossom Sorry for the delay in getting back to you. I'm happy w/ this aside from the requested change.

Cheers for actioning the TODO stuff for me - you did it as it was wanted 😄

Once you've made the requested change, I'll merge this, and then someone will do a release.

@chrisblossom
Copy link
Contributor Author

Thanks @G-Rath for the response! I think I've made the final requested change. If there is anything else please let me know.

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 26, 2019

@chrisblossom sorry I forgot about this 😬

I'm prepping a next branch for release right now, so if you could find some time in the next couple of days to resolve the conflict, that'd be great, as I can then pull it from master into next :)

@SimenB SimenB changed the base branch from master to next October 27, 2019 21:25
@SimenB
Copy link
Member

SimenB commented Oct 27, 2019

resolved the conflict, made breaking, and changed to target next. if CI is happy I'll merge into next

@SimenB SimenB merged commit 1cac8f2 into jest-community:next Oct 27, 2019
SimenB pushed a commit that referenced this pull request Oct 27, 2019
…#306)

BREAKING CHANGE: Rename `require-tothrow-message` to `require-to-throw-message`

closes #296
G-Rath pushed a commit that referenced this pull request Oct 27, 2019
…#306)

BREAKING CHANGE: Rename `require-tothrow-message` to `require-to-throw-message`

closes #296
@chrisblossom chrisblossom deleted the rename-require-tothrow-message branch October 27, 2019 22:58
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.

None yet

4 participants