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.

Sign up for GitHub

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

Jump to bottom

Add autofix to selector-attribute-quotes #5248

Merged
merged 5 commits into from Apr 23, 2021
Merged

Add autofix to selector-attribute-quotes #5248

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2ae703
Add autofix to selector-attribute-quotes
doing-art Apr 14, 2021
fd2596c
Cover autofix of selector-attribute-quotes rule with unit tests
doing-art Apr 14, 2021
ce7575e
Cover autofix of selector-attribute-quotes rule with additional tests
doing-art Apr 15, 2021
d022e8a
Update documentation of selector-attribute-quotes rule
doing-art Apr 15, 2021
24f89d9
Add getRuleSelector util
doing-art Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/rules/selector-attribute-quotes/README.md
View file
Open in desktop
Expand Up @@ -9,6 +9,8 @@ Require or disallow quotes for attribute values.
* These quotes */
```

The [`fix` option](../../../docs/user-guide/usage/options.md#fix) can automatically fix most of the problems reported by this rule.

## Options

`string`: `"always"|"never"`
Expand Down
86 changes: 86 additions & 0 deletions lib/rules/selector-attribute-quotes/__tests__/index.js
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ testRule({
ruleName,
config: ['always'],
skipBasicChecks: true,
fix: true,

accept: [
{
Expand Down Expand Up @@ -54,51 +55,87 @@ testRule({
code: 'html { --custom-property-set: {} }',
description: 'custom property set in selector',
},
{
code: `a[href="te's't"] { }`,
description: 'double-quoted attribute contains single quote',
},
{
code: `a[href='te"s"t'] { }`,
description: 'single-quoted attribute contains double quote',
},
],

reject: [
{
code: 'a[title=flower] { }',
fixed: 'a[title="flower"] { }',
message: messages.expected('flower'),
line: 1,
column: 9,
},
{
code: 'a[ title=flower ] { }',
fixed: 'a[ title="flower" ] { }',
message: messages.expected('flower'),
line: 1,
column: 10,
},
{
code: '[class^=top] { }',
fixed: '[class^="top"] { }',
message: messages.expected('top'),
line: 1,
column: 9,
},
{
code: '[class ^= top] { }',
fixed: '[class ^= "top"] { }',
message: messages.expected('top'),
line: 1,
column: 11,
},
{
code: '[frame=hsides i] { }',
fixed: '[frame="hsides" i] { }',
message: messages.expected('hsides'),
line: 1,
column: 8,
},
{
code: '[data-style=value][data-loading] { }',
fixed: '[data-style="value"][data-loading] { }',
message: messages.expected('value'),
line: 1,
column: 13,
},
{
code: `[href=te\\'s\\"t] { }`,
fixed: `[href="te's\\"t"] { }`,
message: messages.expected(`te's"t`),
line: 1,
column: 7,
},
{
code: '[href=\\"test\\"] { }',
fixed: '[href="\\"test\\""] { }',
message: messages.expected('"test"'),
line: 1,
column: 7,
},
{
code: "[href=\\'test\\'] { }",
fixed: `[href="'test'"] { }`,
message: messages.expected("'test'"),
line: 1,
column: 7,
},
],
});

testRule({
ruleName,
config: ['never'],
fix: true,

accept: [
{
Expand All @@ -116,63 +153,112 @@ testRule({
{
code: '[data-style=value][data-loading] { }',
},
{
code: `a[href=te\\'s\\"t] { }`,
description: 'attribute contains inner quotes',
},
{
code: '[href=\\"test\\"] { }',
description: 'escaped double-quotes are not considered as framing quotes',
},
{
code: "[href=\\'test\\'] { }",
description: 'escaped single-quotes are not considered as framing quotes',
},
],

reject: [
{
code: 'a[target="_blank"] { }',
fixed: 'a[target=_blank] { }',
message: messages.rejected('_blank'),
line: 1,
column: 10,
},
{
code: 'a[ target="_blank" ] { }',
fixed: 'a[ target=_blank ] { }',
message: messages.rejected('_blank'),
line: 1,
column: 11,
},
{
code: '[class|="top"] { }',
fixed: '[class|=top] { }',
message: messages.rejected('top'),
line: 1,
column: 9,
},
{
code: '[class |= "top"] { }',
fixed: '[class |= top] { }',
message: messages.rejected('top'),
line: 1,
column: 11,
},
{
code: "[title~='text'] { }",
fixed: '[title~=text] { }',
message: messages.rejected('text'),
line: 1,
column: 9,
},
{
code: "[data-attribute='component'] { }",
fixed: '[data-attribute=component] { }',
message: messages.rejected('component'),
line: 1,
column: 17,
},
{
code: '[frame="hsides" i] { }',
fixed: '[frame=hsides i] { }',
message: messages.rejected('hsides'),
line: 1,
column: 8,
},
{
code: "[frame='hsides' i] { }",
fixed: '[frame=hsides i] { }',
message: messages.rejected('hsides'),
line: 1,
column: 8,
},
{
code: "[data-style='value'][data-loading] { }",
fixed: '[data-style=value][data-loading] { }',
message: messages.rejected('value'),
line: 1,
column: 13,
},
{
code: `[href="te'st"] { }`,
fixed: "[href=te\\'st] { }",
message: messages.rejected("te'st"),
line: 1,
column: 7,
},
{
code: `[href='te"st'] { }`,
fixed: '[href=te\\"st] { }',
message: messages.rejected('te"st'),
line: 1,
column: 7,
},
{
code: "[href='te\\'s\\'t'] { }",
fixed: "[href=te\\'s\\'t] { }",
message: messages.rejected("te's't"),
line: 1,
column: 7,
},
{
code: '[href="te\\"s\\"t"] { }',
fixed: '[href=te\\"s\\"t] { }',
message: messages.rejected('te"s"t'),
line: 1,
column: 7,
},
],
});

Expand Down
36 changes: 27 additions & 9 deletions lib/rules/selector-attribute-quotes/index.js
View file
Open in desktop
Expand Up @@ -15,7 +15,9 @@ const messages = ruleMessages(ruleName, {
rejected: (value) => `Unexpected quotes around "${value}"`,
});

function rule(expectation) {
const acceptedQuoteMark = '"';

function rule(expectation, secondary, context) {
return (root, result) => {
const validOptions = validateOptions(result, ruleName, {
actual: expectation,
Expand All @@ -36,25 +38,41 @@ function rule(expectation) {
}

parseSelector(ruleNode.selector, result, ruleNode, (selectorTree) => {
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
parseSelector(ruleNode.selector, result, ruleNode, (selectorTree) => {
parseSelector(getRuleSelector(ruleNode), result, ruleNode, (selectorTree) => {

We'll need a new util here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doing-art Thank you for the pull request. It's almost there.

Can we add a test with a comment within a selector list please, e.g.:

{
  code: 'a[target="_blank"], /* comment */ a { }',
  fixed: 'a[target=_blank], /* comment */ a { }',
  message: messages.rejected('_blank'),
  line: 1,
  column: 10,
},

It'll fail because comments are stripped for the selector property of the AST. The full string is available in the raws property, and we'll want to pass this full string to the selector parser. (Ref).

Let's create a getRuleSelector util based off getDeclarationValue and use it in the rule.

@jeddy3 Thanks for reviewing the PR. I made the requested change.

let selectorFixed = false;

selectorTree.walkAttributes((attributeNode) => {
if (!attributeNode.operator) {
return;
}

if (!attributeNode.quoted && expectation === 'always') {
complain(
messages.expected(attributeNode.value),
attributeNode.sourceIndex + attributeNode.offsetOf('value'),
);
if (context.fix) {
selectorFixed = true;
attributeNode.quoteMark = acceptedQuoteMark;
} else {
complain(
messages.expected(attributeNode.value),
attributeNode.sourceIndex + attributeNode.offsetOf('value'),
);
}
}

if (attributeNode.quoted && expectation === 'never') {
complain(
messages.rejected(attributeNode.value),
attributeNode.sourceIndex + attributeNode.offsetOf('value'),
);
if (context.fix) {
selectorFixed = true;
attributeNode.quoteMark = null;
} else {
complain(
messages.rejected(attributeNode.value),
attributeNode.sourceIndex + attributeNode.offsetOf('value'),
);
}
}
});

if (selectorFixed) {
ruleNode.selector = selectorTree.toString();
}
});

function complain(message, index) {
Expand Down