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 ignoreAtRules option to no-invalid-position-at-import #5520

Conversation

sylwia-lask
Copy link
Contributor

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

#5492

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

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.

@sylwia-lask Thanks for another pull request!

I've requested some changes (as suggestions, so you can commit them via the GitHub interface).

They align the docs and tests with our conventions, mainly using standard CSS and consistent names. There's a lot of rules in Stylelint, so we do our best to keep things consistent to help our users move through all the documentation.

lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
@jeddy3
Copy link
Member

jeddy3 commented Sep 12, 2021

I'm going to create a follow-up issue to update all our custom at-rule, function etc documentation to use dashed idents to be more aligned with the specs (see example 7), where it advocates for @--custom rather than @custom.

sylwia-lask and others added 3 commits September 14, 2021 20:39
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
@sylwia-lask
Copy link
Contributor Author

@sylwia-lask Thanks for another pull request!

I've requested some changes (as suggestions, so you can commit them via the GitHub interface).

They align the docs and tests with our conventions, mainly using standard CSS and consistent names. There's a lot of rules in Stylelint, so we do our best to keep things consistent to help our users move through all the documentation.

@jeddy3 thanks a million for the code review :) I fixed everything, hope now everything is ok.

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 a million for the code review

No worries. Thanks for making the changes and for the pull request!

I've requested just two more changes, then I think we're good.

@@ -27,6 +40,10 @@ function rule(actual) {
root.walk((node) => {
const nodeName = node.name && node.name.toLowerCase();

if (node.type === 'atrule' && optionsMatches(options, 'ignoreAtRules', node.name)) {
Copy link
Member

@jeddy3 jeddy3 Sep 14, 2021

Choose a reason for hiding this comment

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

Shall we move this into the conditional below to keep everything together, i.e.:

if (
	(node.type === 'atrule' &&
		nodeName !== 'charset' &&
		nodeName !== 'import' &&
		!optionsMatches(options, 'ignoreAtRules', node.name) &&
		isStandardSyntaxAtRule(node)) ||
	(node.type === 'rule' && isStandardSyntaxRule(node))
) {
	invalidPosition = true;

	return;
}

You're right to use node.name rather than nodeName as the ignore* options are case sensitive, and people can use a regex with the i flag if they want case insensitivity.


Incidentally, I wonder whether this rule can be refactored to avoid walking every node. Instead, walking just import at rules, and whether that'll affect performance or not. One for another time, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, maybe it's worth to refactor that rule. For now I've changed it as you suggest - put every contition together

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, thanks!

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.

LGTM, thank you! 👍🏼

@jeddy3 jeddy3 merged commit 3058f45 into stylelint:v14 Sep 15, 2021
@jeddy3
Copy link
Member

jeddy3 commented Sep 15, 2021

  • Added: ignoreAtRules: [] to no-invalid-position-at-import (#5520).

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 ignoreAtRules: [] to no-invalid-position-at-import-rule
3 participants