Skip to content

Add equality checks to prefer-t-regex rule #297

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

Closed
wants to merge 3 commits into from

Conversation

JaniM
Copy link

@JaniM JaniM commented Feb 28, 2020

Fixes #275.

These are the maximal reasonable rules I could think of.

image


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@JaniM JaniM force-pushed the better-prefer-t-regex branch from f4c109e to c18bc37 Compare March 2, 2020 16:37
@@ -62,6 +62,26 @@ ruleTester.run('prefer-t-regex', rule, {
code: header + 'const reg = /\\d+/;\ntest(t => t.true(reg.test(foo.bar())));',
output: header + 'const reg = /\\d+/;\ntest(t => t.regex(foo.bar(), reg));',
errors: errors('regex')
},
{
code: header + 'test(t => t.is(foo(), /\\d+/));',
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add version of these tests using new RegExp()?

Copy link
Author

Choose a reason for hiding this comment

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

Do we want to expand the functionality for this? The original regex detection logic only matches regex literals, not expressions resolving to a regex. Resolving whether or not any given expression resolves to a regex pretty much requires eval, so we'd have to special-case new RegExp().

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to resolve expression, but we should support an inline new RegExp() call.

@JaniM
Copy link
Author

JaniM commented Mar 30, 2020

Due to the current situation, I don't have time to finish this. Anyone can pick this up.

@novemberborn
Copy link
Member

Thanks for the update @JaniM. Good luck ❤️

@sindresorhus
Copy link
Member

#275 (comment)

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.

Detect more assertions in the t-prefer-regex rule
3 participants