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 new rule: no-comment-textnodes #616

Merged
merged 1 commit into from
Jun 5, 2016
Merged

Add new rule: no-comment-textnodes #616

merged 1 commit into from
Jun 5, 2016

Conversation

benvinegar
Copy link
Contributor

Our team has run into issues where people mistakenly commit comments as text nodes, and they have managed to get through code review and appeared (embarrassingly) in our UI. I threw together this rule to try and catch such occasions, and thought it might be useful to other users of this ESLint plugin.

Feedback, comments greatly appreciated.

@ljharb
Copy link
Member

ljharb commented May 31, 2016

What about false positives where the literal JS comment text is what's desired?

@benvinegar
Copy link
Contributor Author

@ljharb – They have two choices. Either disable the rule, or do:

<div>{'// Not a comment'}</div>

I personally think that's acceptable. This strikes me as a particularly rare edge case.

@ljharb
Copy link
Member

ljharb commented May 31, 2016

@benvinegar want to add documentation for the rule, and include that info in it explicitly? :-D

@benvinegar
Copy link
Contributor Author

@ljharb – Updated with documentation.

@coveralls
Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage increased (+0.01%) to 96.783% when pulling 6707132 on benvinegar:no-comment-textnodes into 82b3aa9 on yannickcr:master.

@coveralls
Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage increased (+0.01%) to 96.783% when pulling 2e27558 on benvinegar:no-comment-textnodes into 82b3aa9 on yannickcr:master.

@coveralls
Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage increased (+0.01%) to 96.783% when pulling 2e27558 on benvinegar:no-comment-textnodes into 82b3aa9 on yannickcr:master.

@yannickcr yannickcr merged commit aa359fb into jsx-eslint:master Jun 5, 2016
@yannickcr
Copy link
Member

Seems to be a useful rule. Merged, thanks!

@remitbri
Copy link

a bite late, but: shouldn't the name of the rule be prefixed with jsx-?

@MoOx
Copy link
Contributor

MoOx commented Jul 8, 2016

I agree that this rule should have been prefixed with jsx-.

@ljharb
Copy link
Member

ljharb commented Jul 8, 2016

No reason we couldn't change that as part of the next major bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants