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

[New] jsx-no-literals: add noAttributeStrings option #2782

Merged
merged 1 commit into from Sep 16, 2020

Conversation

TaLeaMonet
Copy link
Contributor

@TaLeaMonet TaLeaMonet commented Sep 2, 2020

Closes #2496. Fixes #2486.

Co-authored-by: TaLea Carpenter <taleacarpenter@gmail.com>
Co-authored-by: tanmoyopenroot <tanmoy.openroot@gmail.com>

Fixes #jsx-eslint#2486
}
},

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unnecessary new line

Copy link
Member

Choose a reason for hiding this comment

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

+1, altho if it helps readability it'd be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New line removed.

@ljharb ljharb changed the title [Fix] support jsx-no-literals on specific attributes [New] support jsx-no-literals on specific attributes Sep 5, 2020

* `noStrings` (default: `false`) - Enforces no string literals used as children, wrapped or unwrapped.
* `allowedStrings` - An array of unique string values that would otherwise warn, but will be ignored.
* `ignoreProps` (default: `false`) - When `true` the rule ignores literals used in props, wrapped or unwrapped.
* `noAttributeStrings` (default: `false` ) - Enforces no string literals used in attributes when set to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `noAttributeStrings` (default: `false` ) - Enforces no string literals used in attributes when set to `true`.
* `noAttributeStrings` (default: `false`) - Enforces no string literals used in attributes when set to `true`.

additionalProperties: false
}]
},

create(context) {
const defaults = {noStrings: false, allowedStrings: [], ignoreProps: false};
const defaults = {
noStrings: false, allowedStrings: [], ignoreProps: false, noAttributeStrings: false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
noStrings: false, allowedStrings: [], ignoreProps: false, noAttributeStrings: false
noStrings: false,
allowedStrings: [],
ignoreProps: false,
noAttributeStrings: false

when the entire object doesn't fit on one line, each property should be on its own line.

Comment on lines 60 to 62
function defaultMessage() {
switch (true) {
case config.noAttributeStrings:
Copy link
Member

Choose a reason for hiding this comment

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

please use an if with early returns here instead of a switch.


context.report({
node,
message: `${errorMessage}: ${context.getSourceCode().getText(node).trim()}`
message: `${errorMessage}: "${context.getSourceCode().getText(node).trim()}"`
Copy link
Member

Choose a reason for hiding this comment

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

the curly quotes here are quite intentional; straight quotes are never appropriate in prose. please revert this line.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM pending comments

@@ -26,16 +26,17 @@ var Hello = <div>

## Rule Options

There are two options:
There are three options:
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to remove the count, to avoid having to update this in the future (also you're changing it to "three" but i think there's 4 now?):

Suggested change
There are three options:
The supported options are:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, the wording has been changed.

}
},

Copy link
Member

Choose a reason for hiding this comment

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

+1, altho if it helps readability it'd be fine

&& parent.type !== 'JSXAttribute';

function isParentNodeStandard() {
if (!/^[\s]+$/.test(node.value) && typeof node.value === 'string' && parent.type.indexOf('JSX') !== -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!/^[\s]+$/.test(node.value) && typeof node.value === 'string' && parent.type.indexOf('JSX') !== -1) {
if (!/^[\s]+$/.test(node.value) && typeof node.value === 'string' && parent.type.includes('JSX')) {

@ljharb ljharb changed the title [New] support jsx-no-literals on specific attributes [New] jsx-no-literals: add noAttributeStrings option Sep 16, 2020
@ljharb ljharb merged commit 153eac8 into jsx-eslint:master Sep 16, 2020
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.

support jsx-no-literals on specific attributes
4 participants