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 no-invalid-position-at-import-rule #5202

Merged
merged 10 commits into from Mar 26, 2021

Conversation

@chuuddo
Copy link
Contributor

@chuuddo chuuddo commented Mar 18, 2021

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

Closes #5133.

Is there anything in the PR that needs further explanation?

No, it's self-explanatory.

Copy link
Member

@jeddy3 jeddy3 left a comment

@chuuddo Thank you for your first contribution. The pull request is looking good! It's great to see the naming conventions being followed too.

I've made some requests.

Loading

docs/user-guide/rules/list.md Outdated Show resolved Hide resolved
Loading
lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
Loading
lib/rules/no-invalid-position-at-import-rule/index.js Outdated Show resolved Hide resolved
Loading
Loading
Loading
@chuuddo chuuddo requested a review from jeddy3 Mar 21, 2021
Copy link
Member

@jeddy3 jeddy3 left a comment

Thanks for making the changes!

One last request, then it looks good to me.

Loading

Loading
Co-authored-by: Richard Hallows <jeddy3@users.noreply.github.com>
jeddy3
jeddy3 approved these changes Mar 21, 2021
Copy link
Member

@jeddy3 jeddy3 left a comment

Thanks! LGTM.

Loading

Copy link
Contributor

@voskresla voskresla left a comment

thx!

Loading

Copy link
Member

@ybiquitous ybiquitous left a comment

LGTM! 👍
I've left some trivial comments, but this PR is sufficient. 😄

Loading

lib/rules/no-invalid-position-at-import-rule/README.md Outdated Show resolved Hide resolved
Loading
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) {
return;
}

if (node.type === 'atrule' && /^import$/i.test(node.name)) {
Copy link
Member

@ybiquitous ybiquitous Mar 23, 2021

Choose a reason for hiding this comment

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

[IMO] It seems a bit readable to me if using toLowerCase. But this is just my opinion, so you can ignore it. 😃

Suggested change
if (node.type === 'comment' || (node.type === 'atrule' && /^charset$/i.test(node.name))) {
return;
}
if (node.type === 'atrule' && /^import$/i.test(node.name)) {
const nodeName = node.name.toLowerCase();
if (node.type === 'comment' || (node.type === 'atrule' && nodeName === 'charset')) {
return;
}
if (node.type === 'atrule' && nodeName === 'import') {

Loading

Copy link
Contributor Author

@chuuddo chuuddo Mar 23, 2021

Choose a reason for hiding this comment

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

Suggestion looks suitable. Just made some research and toLowerCase() used much more often in rules.
@jeddy3 what do you think about it? Do we have any convention on this or should create one if not?

Loading

Copy link
Member

@jeddy3 jeddy3 Mar 23, 2021

Choose a reason for hiding this comment

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

I had the same thought about toLowerCase as @ybiquitous when I was first reviewing the pull request as we've used that approach in loads of places. However, the regex approach sort of grew on me because we use it for filtering at-rules and declarations when walking (like we did in the last rule we added), so I didn't mention it.

Do we have any convention on this or should create one if not?

I'm all for conventions, especially within rules, because they're such a time saver when reviewing code.

I'm not sure which of the approaches is more readable, though. So, let's go with @ybiquitous's toLowerCase suggestion for now to be consistent with how we're treating names and values in other rules. We can then, if we want, refactor across the board later down the line.

Loading

Copy link
Member

@jeddy3 jeddy3 Mar 23, 2021

Choose a reason for hiding this comment

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

Following on, we'll probably stick with toLowerCase (for names and values) because (having looked myself) we often:

  • do more string manipulation at the same time, e.g. trimming and slicing
  • look for matches in arrays and sets

Loading

chuuddo and others added 2 commits Mar 23, 2021
@jeddy3 jeddy3 merged commit 3dfa452 into stylelint:master Mar 26, 2021
13 checks passed
Loading
@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Mar 26, 2021

Changelog entry added:

  • Added: no-invalid-position-at-import-rule rule (#5202).

Loading

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Apr 27, 2021

@jeddy3 just wondering, should this handle scss files too? Trying to figure out where I should report any issues https://github.com/twbs/stylelint-config-twbs-bootstrap/pull/110/checks?check_run_id=2447711414

Loading

@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Apr 27, 2021

@XhmikosR What SCSS code is triggering those errors?

Loading

@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented Apr 27, 2021

I think we can rejig the rule so that it ignores non-standard syntax (like dollar variables) while being true to the spec that the rule enforces:

Any @import rules must precede all other valid at-rules and style rules in a style sheet (ignoring @charset), or else the @import rule is invalid. [Emphasis mine]

@XhmikosR Can you create a bug ticket here in stylelint, please?

Loading

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Apr 27, 2021

Done, see #5263. Thanks :)

Loading

@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented May 1, 2021

@XhmikosR Fixed in 13.13.1.

Loading

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented May 1, 2021

Thanks!

Loading

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented May 1, 2021

@jeddy3 should this also be fixed here?

Sorry for not catching this earlier, I just updated to the latest versions.

Loading

@jeddy3
Copy link
Member

@jeddy3 jeddy3 commented May 1, 2021

should this also be fixed here?

Nope. That's the pattern the rule is warning against.

I recommend moving that code into another file and importing it, e.g. @import reset, if you'd like to closer align with the CSS specs.

Alternatively, if you don't want to align, you can use /* stylelint-disable no-invalid-position-at-import-rule */ to disable the rule for that section of code, or turn off the rule entirely.

Loading

@ybiquitous
Copy link
Member

@ybiquitous ybiquitous commented May 2, 2021

I agree with @jeddy3.

Besides, it looks like out of this PR scope, but it seems good to me to migrate @import to @use because:

The Sass team discourages the continued use of the @import rule.

-- https://sass-lang.com/documentation/at-rules/import

Loading

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented May 2, 2021

@jeddy3 Thanks, I'll just ignore the warnings in the v4-dev branch 👍

@ybiquitous That's a different thing which we can't do for v4 because it's a breaking change.

Loading

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

Successfully merging this pull request may close these issues.

5 participants