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

Add string-content rule #496

Merged
merged 25 commits into from
Mar 8, 2020
Merged

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Jan 3, 2020

fixes #327


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


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

@fisker fisker force-pushed the rule/string-content branch from 0ed7ffa to 083bda0 Compare January 3, 2020 11:37
@fisker fisker force-pushed the rule/string-content branch 9 times, most recently from d6c5331 to 98abe3e Compare January 7, 2020 10:02
@fisker fisker force-pushed the rule/string-content branch from 98abe3e to f3f6807 Compare January 7, 2020 10:05
@sindresorhus
Copy link
Owner

You're not waiting for feedback from me on anything here, right?

@fisker
Copy link
Collaborator Author

fisker commented Feb 14, 2020

I'm not, but I have problem with this, harder than I thought. I'll explain later when I need your help.

@fisker fisker force-pushed the rule/string-content branch 3 times, most recently from a3af945 to 0d36d5f Compare February 15, 2020 12:39
@fisker fisker force-pushed the rule/string-content branch from 0d36d5f to db71fc0 Compare February 15, 2020 12:45
@fisker fisker force-pushed the rule/string-content branch from 5b373b7 to cb7003f Compare February 15, 2020 13:06
@fisker
Copy link
Collaborator Author

fisker commented Feb 16, 2020

@sindresorhus Set default to off, or we just disable this rule on our codebase?

I suggest set default to off.

@sindresorhus
Copy link
Owner

Yes, it should be off by default. I will enable it in XO with some useful patterns, but I think that's too opinionated for this general plugin.

@fisker fisker changed the title [WIP] Add string-content rule Add string-content rule Feb 16, 2020
@fisker
Copy link
Collaborator Author

fisker commented Feb 16, 2020

@sindresorhus This PR is ready for review.

Something I need explain first, I set this rule only report one error a time on one string node, for the following reasons.

  1. Case: String ab, replacement a -> A, b -> B

If we check all patterns on the original string, we will get two problem with 2 fixes, so we get fix ab -> Ab and ab -> aB, but they are conflict, so only b->B get replaced.

  1. Case: String a, replacement a -> b, b -> c

If we check a -> b first, got result b, then check b -> c on b, got correct result c, we report two problems a -> b, b -> c, but there is no b in the original string.

  1. If we consider some replacement set fix:false, this get even complicated.

Another thing, may need discuss, I run check on TemplateElement instead of TemplateLiteral, so const foo = `a${'a'}a${'a'}a` with a->b will got 3 error on TemplateElement and 2 error on Literal, I'm not sure is this a problem.

@fisker fisker marked this pull request as ready for review February 16, 2020 07:05
readme.md Outdated Show resolved Hide resolved
docs/rules/string-content.md Outdated Show resolved Hide resolved
rules/string-content.js Outdated Show resolved Hide resolved
docs/rules/string-content.md Show resolved Hide resolved
@sindresorhus
Copy link
Owner

If we check all patterns on the original string, we will get two problem with 2 fixes

You can give the report() method an array of fixes. Does that help?

An array which includes fixing objects. - https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

@fisker
Copy link
Collaborator Author

fisker commented Mar 3, 2020

You can give the report() method an array of fixes. Does that help?

I've tried this before, not useful for this case.

@sindresorhus
Copy link
Owner

Alright. Let's at least document that it only reports one thing per node.

@sindresorhus
Copy link
Owner

@fisker I think there's only https://github.com/sindresorhus/eslint-plugin-unicorn/pull/496/files#r386895029 left and this is ready to be merged?

@fisker
Copy link
Collaborator Author

fisker commented Mar 8, 2020

@sindresorhus I've add a simple description 9c0953d . Not sure what else you want add here. Maybe you can help, I'm really bad at docs .

@sindresorhus sindresorhus merged commit 0972a89 into sindresorhus:master Mar 8, 2020
@fisker fisker deleted the rule/string-content branch March 8, 2020 16:11
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.

Rule proposal: string-content
2 participants