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] `jsx-no-literals`: trim whitespace for `allowedStrings` check #2436

Merged
merged 1 commit into from Oct 16, 2019

Conversation

@cainlevy
Copy link
Contributor

commented Oct 2, 2019

The allowedStrings option for jsx-no-literals is checked against an untrimmed string containing potential spaces and newlines. This is at odds with the error message shown and substantially increases the difficulty of managing the allowedStrings list.

I propose trimming strings before checking inclusion in allowedStrings.

related:

const isNoStrings = context.options[0] ? context.options[0].noStrings : false;
const allowedStrings = context.options[0] ? new Set(context.options[0].allowedStrings) : false;
const allowedStrings = context.options[0] ?
new Set((context.options[0].allowedStrings || []).map(trimIfString)) :

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 3, 2019

Collaborator

why the ternary? just making this unconditionally be a Set simplifies the check below

}
}
`,
options: [{noStrings: true, allowedStrings: [' ']}]

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 3, 2019

Collaborator

could we add a few more tests, one with leading and trailing spaces on allowedStrings, and a different one with them on the rendered text?

This comment has been minimized.

Copy link
@cainlevy

cainlevy Oct 4, 2019

Author Contributor

apologies, i'm not sure how to action this request. could you say it another way?

for context, we happen to have a pre-existing test with spaces in allowedStrings that would fail if allowedStrings were not trimmed. here are all the combinations of spaces and not spaces with test coverage:

tested spaces in allowedStrings spaces in rendered text
🚫
🚫 🚫
🚫 🚫

is that the test gap you'd like me to fix?

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 8, 2019

Collaborator

Yes, exactly right :-)

This comment has been minimized.

Copy link
@cainlevy

cainlevy Oct 9, 2019

Author Contributor

Got it!

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 12, 2019

Collaborator

I'd also like a test with noStrings false, and an allowedStrings value :-)

This comment has been minimized.

Copy link
@cainlevy

cainlevy Oct 14, 2019

Author Contributor

I've made noStrings: false explicit in the existing test case

@ljharb
ljharb approved these changes Oct 16, 2019
@ljharb ljharb merged commit 7fa31df into yannickcr:master Oct 16, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 97.537%
Details
@cainlevy cainlevy deleted the cainlevy:jsx-no-literals-trim branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.