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.

Sign up for GitHub

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

Jump to bottom

Catch rule execution exceptions #5012

Closed
wants to merge 1 commit into from
Closed

Catch rule execution exceptions #5012

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 21 additions & 10 deletions lib/lintPostcssResult.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,16 +107,27 @@ function lintPostcssResult(stylelintOptions, postcssResult, config) {

performRules.push(
Promise.all(
postcssRoots.map((postcssRoot) =>
ruleFunction(primaryOption, secondaryOptions, {
fix:
stylelintOptions.fix &&
// Next two conditionals are temporary measures until #2643 is resolved
isFileFixCompatible &&
!postcssResult.stylelint.disabledRanges[ruleName],
newline,
})(postcssRoot, postcssResult),
),
postcssRoots.map(async (postcssRoot) => {
try {
return await ruleFunction(primaryOption, secondaryOptions, {
fix:
stylelintOptions.fix &&
// Next two conditionals are temporary measures until #2643 is resolved
isFileFixCompatible &&
!postcssResult.stylelint.disabledRanges[ruleName],
newline,
})(postcssRoot, postcssResult);
} catch (ex) {
const fileName =
postcssRoot.source && postcssRoot.source.input
? postcssRoot.source.input.file
: 'unknown file';
Copy link
Member

Choose a reason for hiding this comment

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

When I test this change with --stdin, the message includes undefined instead of unknown file:

$ cat a.sass | bin/stylelint.js --stdin --syntax=sass
Rule indentation threw an unknown exception from undefined

So, can we add test cases about this change? It seems good to me to add them to lib/__tests__/standalone.test.js.


// eslint-disable-next-line no-console
Copy link
Author

Choose a reason for hiding this comment

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

How do we feel about disabling this rule here?

Copy link
Member

Choose a reason for hiding this comment

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

IMO, disabling it here seems no problems. 👍
The current ESLint configuration may be a bit too strict, so it seems good to me to loosen it via the allow option as follows:

{
  "no-console": ["error", { "allow": ["warn", "error"] }]
}

https://github.com/stylelint/eslint-config-stylelint/blob/aecb506259e5371e162ee590b67d671d1bc72095/.eslintrc.js#L33

See also https://eslint.org/docs/rules/no-console#options

Copy link
Member

Choose a reason for hiding this comment

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

About this, I've opened a new issue: stylelint/eslint-config-stylelint#108

console.error(`Rule ${ruleName} threw an unknown exception from ${fileName}`);
Copy link
Member

Choose a reason for hiding this comment

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

[IMO] I have some suggestions about this message:

  • How about quoting a rule name?
  • How about quoting a file name?
  • How about using <input css> if a file name is not present?

For example:

Rule "indentation" threw an unknown exception from "/home/foo/button.sass"

Rule "indentation" threw an unknown exception from <input css>

See also:

stylelint/lib/createPartialStylelintResult.js

Line 81 in f0b5aa8

source: cssSyntaxError.file || '<input css 1>',

throw ex;
}
}),
),
);
});
Expand Down