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 function-no-unknown #5865

Merged
merged 16 commits into from Feb 7, 2022
Merged

Add function-no-unknown #5865

merged 16 commits into from Feb 7, 2022

Conversation

gnuletik
Copy link
Contributor

@gnuletik gnuletik commented Jan 25, 2022

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

Closes #5853

Is there anything in the PR that needs further explanation?

"No, it's self-explanatory."

@gnuletik
Copy link
Contributor Author

There's an issue on Windows builds when loading css-functions-list:

ENOENT: no such file or directory, open '/D:/a/stylelint/stylelint/node_modules/css-functions-list/cjs/index.json'

Does someone use Windows and have an idea to fix it?

@ybiquitous
Copy link
Member

@gnuletik How about using URL as below?

fs.readFileSync(new URL("file://" + listPath))

@gnuletik
Copy link
Contributor Author

gnuletik commented Feb 3, 2022

@gnuletik How about using URL as below?

fs.readFileSync(new URL("file://" + listPath))

I just made the change. I need approval to run the CI.

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.

@gnuletik Thanks, almost good! 👍🏼

lib/utils/__tests__/isCustomFunction.test.js Outdated Show resolved Hide resolved
package.json 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.

Thanks, LGTM 👍🏼

@jeddy3 jeddy3 changed the title feat: add function-no-unknown (#5853) Add function-no-unknown Feb 4, 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.

Thanks for the pull request.

I've requested some changes. Let's also add an accept test for TRANSFORM and an reject one for UNKNOWN.

lib/rules/function-no-unknown/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/function-no-unknown/README.md Outdated Show resolved Hide resolved
lib/rules/function-no-unknown/README.md Show resolved Hide resolved
lib/rules/function-no-unknown/README.md Outdated Show resolved Hide resolved
lib/rules/function-no-unknown/__tests__/index.js Outdated Show resolved Hide resolved
lib/rules/function-no-unknown/__tests__/index.js Outdated Show resolved Hide resolved
@gnuletik
Copy link
Contributor Author

gnuletik commented Feb 4, 2022

Thanks @jeddy3 !
I applied your changes, added some test for the if (!isStandardSyntaxFunction) { ... } branch and rebased.

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.

I've requested a couple more.

Let's also add an:

  • accept test for TRANSFORM
  • an reject one for UNKNOWN

So that we test that the rule is case insensitive.


This rule considers functions defined in the CSS Specifications to be known.

This rule ignores double-dashed custom functions, e.g. --custom-function().
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
This rule ignores double-dashed custom functions, e.g. --custom-function().
This rule ignores double-dashed custom functions, e.g. `--custom-function()`.

Minor nit.

Comment on lines 20 to 22
{
code: 'a { $list: (list) }',
},
Copy link
Member

@jeddy3 jeddy3 Feb 4, 2022

Choose a reason for hiding this comment

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

Let's move this SCSS into its own testRule that uses the postcss-scss syntax as we strive to keep standard and non-standard syntax separate, like so.

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.

One last suggestion and I think we're ready to merge.

Comment on lines 48 to 50
ruleName,
config: true,
skipBasicChecks: true,
Copy link
Member

@jeddy3 jeddy3 Feb 6, 2022

Choose a reason for hiding this comment

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

Suggested change
ruleName,
config: true,
skipBasicChecks: true,
ruleName,
config: true,
customSyntax: "postcss-scss",
skipBasicChecks: true,

Let's specify the SCSS syntax (even though the default parser can parse the construct) to be explicit.

@jeddy3 jeddy3 mentioned this pull request Feb 6, 2022
6 tasks
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. LGTM.

@jeddy3 jeddy3 merged commit 6e6bd90 into stylelint:main Feb 7, 2022
@jeddy3
Copy link
Member

jeddy3 commented Feb 7, 2022

Changelog entry:

  • Added: function-no-unknown rule (#5865).

@torhovland
Copy link

torhovland commented Feb 9, 2022

This is now starting to complain when I have

@use 'sass:math';

.service-detail-header-logo__active-icon {
  top: math.div(-$pulse-size, 4);
  ...
Unexpected unknown function "math.div"  function-no-unknown

Although the workaround is to downgrade stylelint-config-standard, not stylelint itself. Hmm...

@ybiquitous
Copy link
Member

@torhovland It is an issue of stylelint-scss: ⬇️

stylelint-scss/stylelint-scss#581

@ybiquitous
Copy link
Member

As a workaround, you can disable the rule like this:

{
  "overrides": [
    {
      "files": ["*.scss", "**/*.scss"],
      "rules": {
        "function-no-unknown": null
      }
    }
  ]
}

@ybiquitous
Copy link
Member

@torhovland #5901 will be released with the next version 14.5.0 (#5902). You can re-enable the rule until stylelint-scss/stylelint-scss#581 is implemented like this:

{
  "overrides": [
    {
      "files": ["*.scss", "**/*.scss"],
      "rules": {
        "function-no-unknown": [true, { "ignoreFunctions": ["math.div"] }]
      }
    }
  ]
}

@kristerkari
Copy link
Contributor

The SCSS version is now also available:
https://github.com/stylelint-scss/stylelint-scss/releases/tag/v4.2.0

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 function-no-unknown
7 participants