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 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 kaicataldo left a comment

Choose a reason for hiding this comment

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

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>;

@kaicataldo kaicataldo added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Feb 17, 2018
@rachx
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
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
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
Copy link
Sponsor 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 kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contributing to ESLint!

@not-an-aardvark not-an-aardvark merged commit aea07dc into eslint:master Feb 23, 2018
@not-an-aardvark
Copy link
Member

Thanks for contributing!

This was referenced Mar 22, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 24, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max-len ignoreStrings does not ignore jsx prop strings
4 participants