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 allowed strings, jsx-no-literals #2380

Merged
merged 1 commit into from Aug 31, 2019

Conversation

@benhollander
Copy link

benhollander commented Aug 12, 2019

No description provided.

Fixes #2377
@benhollander benhollander force-pushed the benhollander:no-literals-allowed-strings branch from 5f78866 to 41bf386 Aug 12, 2019
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
tests/lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
@ljharb ljharb added the enhancement label Aug 15, 2019
@benhollander benhollander force-pushed the benhollander:no-literals-allowed-strings branch from 41bf386 to 7d815b3 Aug 16, 2019
@benhollander

This comment has been minimized.

Copy link
Author

benhollander commented Aug 26, 2019

@ljharb I've updated my branch to reflect the changes you suggested

@ljharb
ljharb approved these changes Aug 31, 2019
@ljharb ljharb force-pushed the benhollander:no-literals-allowed-strings branch from 7d815b3 to 2c6430d Aug 31, 2019
@ljharb ljharb merged commit 2c6430d into yannickcr:master Aug 31, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 97.571%
Details
@benhollander benhollander deleted the benhollander:no-literals-allowed-strings branch Sep 17, 2019
@cainlevy

This comment has been minimized.

Copy link
Contributor

cainlevy commented Sep 30, 2019

I'd love to use this. Is there a timeline for new releases?

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Oct 1, 2019

v7.15.0 is released.

@cainlevy

This comment has been minimized.

Copy link
Contributor

cainlevy commented Oct 2, 2019

@benhollander is this working as you hoped? we're also dealing with   (and other punctuation) but the allowedStrings option seems to be confounded by leading or trailing whitespace.

@benhollander

This comment has been minimized.

Copy link
Author

benhollander commented Oct 2, 2019

@cainlevy it does work for my use case, but I can see that this line doesn't take whitespace into account, since it's doing a straight string comparison.

Options:

  1. We could make that function strip whitespace from node.value before comparing it to the list of strings coming from the config. That would likely result in some results that users might not expect.
  2. We could all for passing a regex instead of a straight string. That seems like it would result in false positives/negatives if people didn't write perfect regexes, which seems likely to me.

Other ideas?

@cainlevy

This comment has been minimized.

Copy link
Contributor

cainlevy commented Oct 2, 2019

  1. My first instinct for the immediate issue is also trim. Can you say more about a situation where it might be surprising?

    It's worth noting here that the error message trims the value, which creates confusion about what should be added to allowedStrings. For example, this error message from my project was not fixed by adding allowedStrings: [" "]:

    Missing JSX expression container around literal string: “ ”         react/jsx-no-literals
    
  2. We're migrating from tslint. The equivalent rule there is jsx-use-translation-function and its options are allow-punctuation and allow-htmlentities. Those options trade abstractness for usability and may take care of most of what people want from allowedStrings and allowedRegexps with less fiddling. Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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