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: Make max-len ignoreStrings ignore JSXText (fixes #9954) #9985

Merged
merged 2 commits into from Feb 23, 2018

Conversation

@rachx
Copy link
Contributor

rachx commented Feb 17, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #9954.

What changes did you make? (Give an overview)
Previously, getAllStrings only filter for tokens with type String. The filter should check if the token type is JSXText as well.

Is there anything you'd like reviewers to focus on?
Nothing in particular.

Copy link
Member

kaicataldo left a comment

Thanks for contributing to ESLint! This looks good to me, but I do have one question.

This change will make it so that text in between tags will also be ignored. Is this the desired behavior? The issue only mentions props, so I just wanted to double check.

Example:

var foo = <div className="this is a very long string">this is another very long string</div>;
@rachx

This comment has been minimized.

Copy link
Contributor Author

rachx commented Feb 18, 2018

I am not sure if it is desired behavior but I think

var foo = <div className="this is a very long string"> short </div>;

should be ignored but

var foo = <div>this is another very long string</div>;

should not be ignored (since this case is more avoidable and we usually expect strings to be in quotes)

How about an additional check if the token is surrounded by quotes?

@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Feb 18, 2018

Looked into this a little more, and given the description of the option ("ignoreStrings": true ignores lines that contain a double-quoted or single-quoted string), I do think we should limit this change to just checking the strings in JSX attributes. If we ever wanted to also check the JSXText between tags, we could always do that in a future PR.

How about an additional check if the token is surrounded by quotes?

I think we might be able to just check if the JSXText token is a child of a JSXAttribute node. What do you think?

@rachx rachx changed the title Fix: Make max-len ignoreStrings ignore JSXText (fixes #9954) Fix: Make max-len ignoreStrings ignore JSXText within JSXAttribute (fixes #9954) Feb 18, 2018
@rachx rachx changed the title Fix: Make max-len ignoreStrings ignore JSXText within JSXAttribute (fixes #9954) Fix: Make max-len ignoreStrings ignore JSXText (fixes #9954) Feb 18, 2018
@rachx

This comment has been minimized.

Copy link
Contributor Author

rachx commented Feb 18, 2018

Updated to check if JSXText token is a child of a JSXAttribute node. I think that will be cleaner than checking if the token's value is surrounded by quotes

@ljharb

This comment has been minimized.

Copy link
Contributor

ljharb commented Feb 18, 2018

I agree with #9985 (comment); a child node is not a string, even tho it's also text.

Copy link
Member

kaicataldo left a comment

LGTM. Thanks for contributing to ESLint!

@not-an-aardvark not-an-aardvark merged commit aea07dc into eslint:master Feb 23, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@not-an-aardvark

This comment has been minimized.

Copy link
Member

not-an-aardvark commented Feb 23, 2018

Thanks for contributing!

This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.