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 ignoreAfterCombinators: [] to selector-max-universal #6275

Merged
merged 10 commits into from Aug 18, 2022

Conversation

fpetrakov
Copy link
Contributor

Which issue, if any, is this issue related to?

Closes #5792

Is there anything in the PR that needs further explanation?

No, it's self-explanatory, I guess.

@changeset-bot

This comment was marked as off-topic.

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@fpetrakov Thanks for creating the PR! Please consider addressing my reviews.

lib/rules/selector-max-universal/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-universal/README.md Outdated Show resolved Hide resolved
lib/rules/selector-max-universal/README.md Outdated Show resolved Hide resolved
@fpetrakov
Copy link
Contributor Author

@ybiquitous I guess, I've added support for user defined combinators. What do you think?

@ybiquitous ybiquitous changed the title Add ignoreAfterCombinators: [] to ignore to selector-max-universal #5792 Add ignoreAfterCombinators: [] to selector-max-universal Aug 17, 2022
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@fpetrakov Thanks. I've left more refactoring suggestions, but it's good enough. LGTM 👍🏼

@jeddy3 I would appreciate it if you would confirm the PR, just in case.

lib/rules/selector-max-universal/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/selector-max-universal/index.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

👍🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@fpetrakov Thanks for the pull request. It's looking good.

I've requested a few changes to the tests and docs, and asked one question about the rule code.


### `ignoreAfterCombinators: ["array", "of", "combinators"]`

Ignore universal selector if it's preceded by specified after combinators.
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
Ignore universal selector if it's preceded by specified after combinators.
Ignore universal selectors that come after one of the specified combinators.

Given:

```json
[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
[1, [">", "+"]]
[">", "+"]

We use this format at the moment, although this may change in the future. Let's make it consistent for now.

Comment on lines 90 to 106
The following patterns are considered problems:

<!-- prettier-ignore -->
```css
*.foo * {}
```

The following patters are _not_ considered problems:

<!-- prettier-ignore -->
```css
*.foo + * {}
```

<!-- prettier-ignore -->
```css
*.foo > * {}
Copy link
Member

@jeddy3 jeddy3 Aug 18, 2022

Choose a reason for hiding this comment

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

Suggested change
The following patterns are considered problems:
<!-- prettier-ignore -->
```css
*.foo * {}
```
The following patters are _not_ considered problems:
<!-- prettier-ignore -->
```css
*.foo + * {}
```
<!-- prettier-ignore -->
```css
*.foo > * {}
For example, with `2`.
The following pattern is _not_ considered a problem:
<!-- prettier-ignore -->
```css
* * > * {}

We generally only show in our docs how ignore options make the rule more permissive. For example.

Comment on lines 272 to 295
testRule({
ruleName,
config: [0, { ignoreAfterCombinators: ['+'] }],

accept: [
{
code: 'foo {}',
},
{
code: 'foo + * {}',
},
],

reject: [
{
code: '* {}',
message: messages.expected('*', 0),
line: 1,
column: 1,
endLine: 1,
endColumn: 2,
},
],
});
Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this into the other config: 0 testRule below.


accept: [
{
code: 'foo {}',
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
code: 'foo {}',
code: 'a {}',

code: 'foo {}',
},
{
code: 'foo + * {}',
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
code: 'foo + * {}',
code: 'a + * {}',

Comment on lines 65 to 70
// checks if previous child node is descendant combinator
if (prevChildNode && prevChildNode.type === 'combinator' && prevChildNodeValue === ' ') {
prevChildNode = prevChildNode.prev();

prevChildNodeValue = prevChildNode && prevChildNode.value;
}
Copy link
Member

Choose a reason for hiding this comment

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

What purpose does this bit of code have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What purpose does this bit of code have?

As you know, there are 4 types of combinators, according to MDN. This code's purpose is to add support for descendant combinator. This way user can add smth like this to the configuration: ["h1"].

For example, we have code like this: a > * { color: red }. With combinators ">", "+", "~" it's very easy to check, in this case we did it this way:
let prevChildNode = childNode.prev();
let prevChildNodeValue = prevChildNode && prevChildNode.value;
prevChildNodeValue will equal to ">", "+" or "~", and then we just pass the value to optionMatches function:
optionsMatches(secondaryOptions, 'ignoreAfterCombinators', prevChildNodeValue)

If we have code h1 * { color: red } and we want to ignore * after h1, we will stumble upon descendant combinator, which is represented by a single space (" ") character. Inn this case, the code from above will not work as prevChildNoveValue will equal to " ". We have to go back even farther and check previous node of the prevChildNode to get the value h1.

Copy link
Member

@jeddy3 jeddy3 Aug 18, 2022

Choose a reason for hiding this comment

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

Thanks for the explanation. I think we can remove this code.

This code's purpose is to add support for the descendant combinator.

I believe the rule supports the descendant combinator without this code, via the configuration of [" "]. We could probably do more to accommodate other whitespace descendant combinators (like tabs and newlines), but let's only do that if someone requests it. By supporting ">", "+", "~" and " " (space) we satisfy the most common use cases and the original one in #5792.

If we have code h1 * { color: red } and we want to ignore * after h1

We can introduce a separate ignoreAfterSelector: [] secondary option to support this as h1 is a type selector, not a combinator (and ["h1"] shouldn't be a valid configuration for the ignoreAfterCombinators: [] option). Let's only add this option if someone requests it, though.

@fpetrakov fpetrakov force-pushed the update-selector-max-universal branch from ef7630f to 67e4c18 Compare August 18, 2022 08:08
@fpetrakov fpetrakov requested a review from jeddy3 August 18, 2022 08:10
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@fpetrakov Thanks for making the changes and for the explanation.

I've requested one last change, otherwise it LGTM.

Comment on lines 65 to 70
// checks if previous child node is descendant combinator
if (prevChildNode && prevChildNode.type === 'combinator' && prevChildNodeValue === ' ') {
prevChildNode = prevChildNode.prev();

prevChildNodeValue = prevChildNode && prevChildNode.value;
}
Copy link
Member

@jeddy3 jeddy3 Aug 18, 2022

Choose a reason for hiding this comment

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

Thanks for the explanation. I think we can remove this code.

This code's purpose is to add support for the descendant combinator.

I believe the rule supports the descendant combinator without this code, via the configuration of [" "]. We could probably do more to accommodate other whitespace descendant combinators (like tabs and newlines), but let's only do that if someone requests it. By supporting ">", "+", "~" and " " (space) we satisfy the most common use cases and the original one in #5792.

If we have code h1 * { color: red } and we want to ignore * after h1

We can introduce a separate ignoreAfterSelector: [] secondary option to support this as h1 is a type selector, not a combinator (and ["h1"] shouldn't be a valid configuration for the ignoreAfterCombinators: [] option). Let's only add this option if someone requests it, though.

@fpetrakov fpetrakov requested a review from jeddy3 August 18, 2022 09:07
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, including adding the test to show that the (single whitespace) descendant combinator value works as a configuration value.

One last revert request, otherwise LGTM.


## Optional secondary options

### `ignoreAfterCombinators: [" ", ">", "*", "~"]`
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
### `ignoreAfterCombinators: [" ", ">", "*", "~"]`
### `ignoreAfterCombinators: ["array", "of", "combinators"]`

We use this format to describe options that accept user-defined lists of things to ignore rather than list out the possible values as:

  • there be many values, e.g. for an option like ignoreUnits: []
  • more of the things being targeted may be added to the language, e.g. new combinators, like the shadow-piercing descendant combinator (>>>)

@fpetrakov fpetrakov requested a review from jeddy3 August 18, 2022 09:24
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

@fpetrakov Thanks again for making the changes.

LGTM.

@ybiquitous Feel free to merge this as part of the upcoming release and trial of the change-set workflow.

@ybiquitous
Copy link
Member

@jeddy3 No problems. The code and doc are much more straightforward than before! 👍🏼

@fpetrakov Thanks again for your contribution! 👏🏼

@ybiquitous ybiquitous merged commit 92d8399 into stylelint:main Aug 18, 2022
@ybiquitous
Copy link
Member

Changelog entry added:

  • Added: ignoreAfterCombinators: [] to selector-max-universal (#6275).

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

Successfully merging this pull request may close these issues.

Add ignoreAfterCombinators: [] to ignore to selector-max-universal
3 participants