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 ignore: ["inside-block"] and splitList to selector-disallowed-list #6334

Conversation

mattmanuel90
Copy link
Contributor

@mattmanuel90 mattmanuel90 commented Sep 10, 2022

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

Closes #6294

Is there anything in the PR that needs further explanation?

A bit of a shuffle, but I added an additional iterateThroughSelectorList flag which iterates through ruleNode.selectors rather than just the single ruleNode.selector or raw.selector.raw. The RegExp can get a bit unwieldy.

I consider the following .foo, .bar {} to be 2 separate selectors, so a dev may prefer to parse each of these individually ( .foo and .bar) than to have to parse a string like .foo,\n.bar

@changeset-bot
Copy link

changeset-bot bot commented Sep 10, 2022

🦋 Changeset detected

Latest commit: c78c764

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jeddy3 jeddy3 changed the title Add ignore: ["inside-block"] and iterateThroughSelectorList option to selector-disallowed-list Add ignore: ["inside-block"] and iterateThrough to selector-disallowed-list Sep 10, 2022
@jeddy3 jeddy3 changed the title Add ignore: ["inside-block"] and iterateThrough to selector-disallowed-list Add ignore: ["inside-block"] and iterateThroughList to selector-disallowed-list Sep 10, 2022
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.

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

Let's drop Selector from the option name to be consistent with other rules. Is iterateThrough the best wording, or can we think of an alternative?

I've requested a few changes (as suggestions that you can commit) to the docs to be consistent with other rules too.

lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/__tests__/index.js Outdated Show resolved Hide resolved
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@mattmanuel90
Copy link
Contributor Author

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

Let's drop Selector from the option name to be consistent with other rules. Is iterateThrough the best wording, or can we think of an alternative?

I've requested a few changes (as suggestions that you can commit) to the docs to be consistent with other rules too.

Thanks for the suggestions. Definitely way more concise.

Between iterateThrough and iterateThroughList, just based on feeling, I think the latter, it's adequate enough when paired with the description in the README, the former might raise an eyebrow.

Will resolve

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.

@mattmanuel90 Thanks for creating the pull request. Adding the iterateThroughList option is a nice idea. 👍🏼

I've left comments about the doc fix and a tiny refactoring. Please consider addressing them.

lib/rules/selector-disallowed-list/README.md Outdated Show resolved Hide resolved
lib/rules/selector-disallowed-list/index.js Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Sep 11, 2022

Between iterateThrough and iterateThroughList, just based on feeling, I think the latter, it's adequate enough when paired with the description in the README, the former might raise an eyebrow.

I meant is there an alternative to "iterate through list" rather than drop "list", for example:

  • splitList - Split a selector list into individual selectors before checking
  • checkSelectorsInList - If true, this rule will check individual selectors in selectors lists.
  • ...

Can we think of any others or are we happy with iterate through? We'll want to get the name right as we'll likely use the option again for other selector-* rules.


Other boolean options we already have include:

@mattmanuel90
Copy link
Contributor Author

mattmanuel90 commented Sep 12, 2022

Ah fair point, I should've factored that in. Ummm.

I’d go with splitSelectorList as my first choice.

I think it’s better to have selectorList than list because selector list is the official term for it, list might be too vague for some and maybe we would want to use this option too in non selector-* rules in the future (just a what-if situation)

Split is also the stronger prefix to use than check (I’d rank split > iterate > check). Split is explicit about separating selectors through a list, whereas check (i.e checkSelectorsInList) isn't acute enough in intention. Dev's might currently have RegExp that already check for selectors in a list.

@jeddy3
Copy link
Member

jeddy3 commented Sep 12, 2022

Split would be my first choice too. Like resolve, it describes the action taken before the thing is checked.

I think it’s better to have selectorList than list because selector list is the official term for it, list might be too vague for some and maybe we would want to use this option too in non selector-* rules in the future (just a what-if situation)

It's a good point. However, let's go with list for consistency with preexisting options. If we do add a similar option to non selector-* and there is ambiguity, we can use selector list there.


Incidentally, I now feel that checkPrefixed was a mistake as it makes the rule stricter (and we generally make rules a stick as possible by default and add options to make them more permissive). The property-no-unknown rule should have checked prefixed properties by default and had an ignore: ["prefixed"] option. It's too late to change it now, but we should try to avoid adding check* options in the future.

@jeddy3 jeddy3 changed the title Add ignore: ["inside-block"] and iterateThroughList to selector-disallowed-list Add ignore: ["inside-block"] and splitList to selector-disallowed-list Sep 12, 2022
@jeddy3 jeddy3 mentioned this pull request Sep 12, 2022
6 tasks
@mattmanuel90 mattmanuel90 requested review from ybiquitous and jeddy3 and removed request for jeddy3 and ybiquitous September 12, 2022 23:49
@mattmanuel90
Copy link
Contributor Author

Sorry ignore the rerequest spam, i think its bugged

@mattmanuel90 mattmanuel90 requested review from ybiquitous and jeddy3 and removed request for ybiquitous September 13, 2022 00:05
mattmanuel90 and others added 2 commits September 13, 2022 10:14
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
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.

@mattmanuel90 Thank you. LGTM 👍🏼

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.

LGTM, thank you.

@jeddy3 jeddy3 merged commit bb10282 into stylelint:main Sep 13, 2022
@mattmanuel90 mattmanuel90 deleted the feature/selector-disallowed-ignore-descendants branch September 13, 2022 11:54
@mattmanuel90 mattmanuel90 restored the feature/selector-disallowed-ignore-descendants branch September 13, 2022 13:15
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 ignore: ["inside-block"] to selector-disallowed-list
3 participants