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 support for reporting with custom severity #6444

Merged

Conversation

aaronccasanova
Copy link
Member

Short description

This PR introduces the ability to report warnings with custom rule severities:

stylelint.utils.report({ ruleName, severity: 'warning' })

Context

My team is working on a rule that is used for both linting and gathering metrics on warnings and have encountered a need to set custom rule severities per report. In short our rule dynamically categorizes warnings and reports with a modified rule name that we target on an external metrics dashboard e.g.

stylelint.utils.report({ ruleName: `${ruleName}/colors`, ... })
stylelint.utils.report({ ruleName: `${ruleName}/layout`, ... })
// etc.

However, because we report dynamically created rule names the warning's severity is set to undefined.

In summary

This small restructure and override mechanism gives us more programmatic control when handling warnings and from what I can tell doesn't affect the CLI or VS Code extension.

That said, I'm very interested to hear what folks think!

@changeset-bot
Copy link

changeset-bot bot commented Nov 2, 2022

🦋 Changeset detected

Latest commit: badb50b

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

@ybiquitous ybiquitous added status: needs discussion triage needs further discussion pr: blocked is blocked by another issue or pr labels Nov 2, 2022
@ybiquitous
Copy link
Member

@aaronccasanova Thanks for requesting the new feature and writing the code.

We usually implement a new feature after a discussion on an issue and reject direct pull requests without a discussion.

However, this feature request seems to make sense, and the code in the pull request is reliable because of enough test cases. Also, I believe adding the optional severity parameter should not break our ecosystem (3rd-party plugins).

But I'm not sure if I'm missing something about the new feature, so I've marked this needs discussion just in case.

@ybiquitous
Copy link
Member

If there are no objections for a while, we can proceed.

@aaronccasanova
Copy link
Member Author

Hello @ybiquitous 👋 Thanks for the quick response! I'd be happy to move this over to an issue and unpack more of the details. We have a pretty unique use case so I'd understand if the proposed changes don't align with the scope of the project and capabilities exposed to 3rd-party plugins.

As an aside, should we update the Code contributions or Open a pull request section of the documentation to state the projects preference for creating issues before pull requests?

@ybiquitous
Copy link
Member

Thanks for the proposal. But these cases aren't very common, so I don't think we need to update the contribution guide for now.

@aaronccasanova
Copy link
Member Author

aaronccasanova commented Nov 5, 2022

I encountered another use case that could leverage the proposed update. I'm interested in locally extending the behavior of the declaration-property-unit-disallowed-list rule to allow consumers to configure severity per property e.g.

// Consumer config
module.exports = {
  plugins: ['@shopify/stylelint-polaris/plugins'],
  rules: {
    'stylelint-polaris/declaration-property-unit-disallowed-list': {
      'font-size': [['em', 'px'], {severity: 'error'}],
      '/^animation/': ['s', {severity: 'warning'}],
    },
  },
}

@ybiquitous ybiquitous removed status: needs discussion triage needs further discussion pr: blocked is blocked by another issue or pr labels Nov 8, 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.

I've removed the blocking labels since there are no objections.

Thanks for your patience. LGTM 👍🏼

@aaronccasanova
Copy link
Member Author

Thanks @ybiquitous! Is there anything else I need to include in this PR (e.g. documentation) or is it ready to ship?

@ybiquitous
Copy link
Member

No need to do anything else. I will merge this in a few days if there are no problems. And we will ship a new version, including this PR, within a few weeks at the latest.

@ybiquitous ybiquitous merged commit 35c0cfa into stylelint:main Nov 9, 2022
@aaronccasanova aaronccasanova deleted the report-with-custom-severity branch November 9, 2022 17:10
@josser
Copy link

josser commented Nov 16, 2022

Not sure but looks like this change introduce some incompatibility with previous configuration.

I.e, if I'm just update stylint, w/o changing my config:

{
  "extends": [
    "stylelint-config-standard",
    "stylelint-config-standard-scss",
    "stylelint-config-prettier",
    "stylelint-config-prettier-scss",
    "stylelint-prettier/recommended"
  ],
  "rules": {
    "selector-pseudo-element-no-unknown": [
      true,
      {
        "ignorePseudoElements": [
          "",
          "ng-deep"
        ]
      }
    ],
    "selector-class-pattern": "^.[a-z]([a-z0-9-]+)?(__([a-z0-9]+-?)+)?(--([a-z0-9]+-?)+){0,2}$",
    "function-no-unknown": [
      true,
      {
        "ignoreFunctions": [
          "-webkit-gradient",
          "from",
          "to"
        ]
      }
    ],
    "selector-type-no-unknown": null
  }
}

Im getting an error:
Error: Unknown severity: "undefined"

@ybiquitous
Copy link
Member

@josser Thanks for the report. Could you provide a minimal reproduction repository, including .stylelintrc.json, package.json, and CSS files?

@josser
Copy link

josser commented Nov 16, 2022

@ybiquitous sure, https://github.com/josser/stylelint-minimal-reproduce

@ybiquitous
Copy link
Member

@josser Thank you so much! I could reproduce the bug, so I'll submit a pull request to fix it. Please be patient. Thanks again. 😄

@ybiquitous
Copy link
Member

As a quick fix, you can avoid the error by fixing your SCSS code like this:

--- a/src/highlight-dialog/highlight-dialog.component.scss
+++ b/src/highlight-dialog/highlight-dialog.component.scss
@@ -89,8 +89,8 @@
   overflow-y: auto;
   height: 200px;
   margin-bottom: 1rem;
-  &// Scroll bar styling
-  ::-webkit-scrollbar {
+  // Scroll bar styling
+  &::-webkit-scrollbar {
     width: 3px;
     height: 3px;
   }

https://github.com/josser/stylelint-minimal-reproduce/blob/e0e045fdc63c4beace2a77e08aa2bde7702fbad1/src/highlight-dialog/highlight-dialog.component.scss#L92

Because the error's cause is "Cannot parse selector". But anyway, I'll fix the severity problem soon.

@josser
Copy link

josser commented Nov 16, 2022

@ybiquitous Thank you! I'm not really good at css/scss, just updating packages :D

@ybiquitous
Copy link
Member

This error also occurs in versions before v14.15.0, so this pull request seems not a cause:

$ npx stylelint --version
14.14.1

$ npm run stylelint:check

> stylelint-error@1.0.0 stylelint:check
> stylelint '**/*.scss'

Error: Unknown severity: "undefined"

@josser
Copy link

josser commented Nov 16, 2022

Hm, but not reproducible in 14.6.0

@aaronccasanova
Copy link
Member Author

aaronccasanova commented Nov 16, 2022

This error also occurs in versions before v14.15.0, so this pull request seems not a cause

Agreed, this seems unrelated. I pulled the example repo and ran the linter programmatically to gain more insight on the cause of the error e.g.

Update: I also cleaned up highlight-dialog.component.scss to only include the relevant warning
.activities {
  &// Scroll bar styling
  ::-webkit-scrollbar {
    width: 3px;
    height: 3px;
  }
}
// node lint-styles.mjs

import stylelint from 'stylelint'

const { results } = await stylelint.lint({
  files: '**/*.scss',
  configFile: '.stylelintrc',
})

console.log(results[0])

/*
Where results[0].warnings === [
  {
    line: 2,
    column: 3,
    endLine: 6,
    endColumn: 4,
    rule: undefined,
    severity: undefined,
    text: 'Cannot parse selector'
  }
]
*/

Searching through stylelint I see that error message referenced in two places (parseSelector.js and transformSelector.js). Do we just need to add a severity: 'error' property to the warning? Or are stylelintType: 'parseError' warnings handled elsewhere?

@ybiquitous
Copy link
Member

@aaronccasanova Thanks for digging into the problem. I found this error is caused by stylelint-scss, not stylelint. See below:

https://github.com/stylelint-scss/stylelint-scss/blob/1c7bc949ea9470a3f471b9670ba41c1946e7ce63/src/utils/parseSelector.js#L7

The parseSelector of stylelint-scss is different from our one: it misses the stylelintType property. So the error by stylelint-scss is not reported as a parserError:

for (const error of result.parseErrors) {

I found other bugs in the formatters, so I've opened PR #6480.

@ybiquitous
Copy link
Member

PR #6480 is merged. The next version will fix the error. Thanks for the report and investigation! @josser @aaronccasanova

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.

None yet

3 participants