Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Added rule+file-specific error messages to rule crashes #3836

Merged
merged 3 commits into from
Apr 26, 2018
Merged

Added rule+file-specific error messages to rule crashes #3836

merged 3 commits into from
Apr 26, 2018

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Apr 16, 2018

PR checklist

Overview of change:

Added the rule and file names with error stack to complaints for crashing rules.

C:\Code\tslinttest>node C:\Code\tslint\bin\tslint --config tslint.json index.ts
The 'ban' rule threw an error in 'index.ts': Error: Hi there!
    at Rule.apply (C:\Code\tslint\lib\rules\banRule.js:33:15)
    at Linter.applyRule (C:\Code\tslint\lib\linter.js:197:29)
    at C:\Code\tslint\lib\linter.js:139:85
    at Object.flatMap (C:\Code\tslint\lib\utils.js:151:29)
    at Linter.getAllFailures (C:\Code\tslint\lib\linter.js:139:32)
    at Linter.lint (C:\Code\tslint\lib\linter.js:99:33)
    at C:\Code\tslint\lib\runner.js:206:32
    at step (C:\Code\tslint\node_modules\tslib\tslib.js:133:27)
    at Object.next (C:\Code\tslint\node_modules\tslib\tslib.js:114:57)
    at fulfilled (C:\Code\tslint\node_modules\tslib\tslib.js:104:62)

Is there anything you'd like reviewers to focus on?

It's not clear where tests for this should live. Would you like me to add some, and if so where?

CHANGELOG.md entry:

[enhancement] Added the rule and file names with error stack to complaints for crashing rules.

It's not clear where tests for this should live. Would you like me to add some, and if so where?
src/error.ts Outdated
@@ -53,3 +53,7 @@ export function showWarningOnce(message: string) {
shownWarnings.add(message);
}
}

export function showRuleCrashWarningOnce(message: string, ruleName: string, fileName: string) {
showWarningOnce(`The '${ruleName}' rule threw an error in '${fileName}': ${message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make the error more readable if the message started in a new line?

@suchanlee
Copy link
Contributor

@JoshuaKGoldberg happy to merge this once the new line mentioned above is added

@JoshuaKGoldberg
Copy link
Contributor Author

Thanks for the ping! Added an endline. Also stopped using the show-once logic because these warnings are unique to rule+file.

@suchanlee suchanlee merged commit 49399eb into palantir:master Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants