-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Don't abort when rule throws, but continue #87
Conversation
da45402
to
cb33fd0
Compare
If a project is being linted and a rule throws an error — perhaps it relies on a file to be in a certain format, or some other reason — then linting for that project will halt, and the lint report for that project will only show that error. An error like this signifies a bug in `module-lint`, but sometimes we can't fix that bug right away, and in the meantime this makes the report useless, since we don't know whether any of the other rules passed or not. This commit fixes this problem by catching errors inside of rules and displaying them as such. To implement this change, we needed to convert the binary status of a rule to a trinary status ("passed", "failed", "errored"). We also updated the output of the lint report to display this error status and distinguish it from a failed status.
cb33fd0
to
ac1954b
Compare
console.error(error); | ||
process.exitCode = 1; | ||
}); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, main
would set process.exitCode
. However, to remain testable, I'm trying to keep main
free of changing global state, so that means changing process
. To accommodate this constraint, I had main
return true
or false
, which we can then use to set process.exitCode
accordingly.
}); | ||
}); | ||
|
||
it('does not exit immediately if a project errors during linting, but shows the error and continues', async () => { | ||
it('does not abort linting of a project if a lint rule throws, but shows the error and continues to the next line rule, returning false', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two similar tests in this file which exercise the new logic added in this PR; this is one of them.
@@ -49,7 +49,7 @@ describe('Rule: all-yarn-modern-files-conform', () => { | |||
}); | |||
|
|||
expect(result).toStrictEqual({ | |||
passed: true, | |||
status: 'passed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All rules return a result object, and that result object contains the status. So the test file for every rule needed to be updated to check for the correct property.
We discussed about errors blocking the completion of module linting. And good to see this PR fixes it. Additionally, there's one more good to have feature (definitely in a separate ticket). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Just one query on the variable renaming.
src/main.ts
Outdated
@@ -231,23 +225,25 @@ async function lintProjects({ | |||
projectLintResultPromiseOutcomes.filter(isPromiseRejectedResult); | |||
|
|||
outputLogger.logToStdout(''); | |||
let isAllPassed = true; | |||
|
|||
let allPassed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious on the renaming? prefixing is
seems to be fine for the boolean!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it because I wanted it to read more like a part of a sentence. So I was thinking of "all passed" like how you'd ask a question ("all passed?"). But maybe that doesn't make sense here. didAllPass
would probably make more sense, but it's kind of awkward. Ah... I can change it back 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, done here: b206993
Hmm. All of the numbers we give at the end are totals, which means that they correspond to rules that we show above. If we show the number of skipped, that implies that we would need to list them. Is there a reason why we or someone would want to know which rules were skipped? Is it just to verify for ourselves that all rules that could have been run were at least considered? Otherwise, I'm not sure that people would think about it — they might only care about passing and failing. |
Sure. My thought process was, for example, deleting package.json can show just one lint failed (package.json does not exist). Once the user fixes it, suddenly it'll start showing couple of more lint errors! which were hidden/didn't execute because there was no package.json. I thought this may surprise the user. But seems like, not that important. They'll know by the type of error/failed list. |
Co-authored-by: Kanthesha Devaramane <kanthesha.devaramane@consensys.net>
Gotcha. Good point. Something to think about for sure. |
The CI failure seems to be happening because we're using a hack to import the Node-specific functions from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
If a project is being linted and a rule throws an error — perhaps it relies on a file to be in a certain format, or some other reason — then linting for that project will halt, and the lint report for that project will only show that error. An error like this signifies a bug in
module-lint
, but sometimes we can't fix that bug right away, and in the meantime this makes the report useless, since we don't know whether any of the other rules passed or not.This commit fixes this problem by catching errors inside of rules and displaying them as such. To implement this change, we needed to convert the binary status of a rule to a trinary status ("passed", "failed", "errored"). We also updated the output of the lint report to display this error status and distinguish it from a failed status.
References
See this recent
module-lint
run for an example of why the output is useless when an error is thrown instead of caught.Fixes #86.
Screenshots
Before
In this example we run
yarn run-tool utils
. Note how no rules are listed at all but rather a single error is shown:In this example we run
yarn run-tool .
(which lintsmodule-lint
itself):After
Running
yarn run-tool utils
. Note how instead of a single error being shown, the statuses of rules are shown and the error that's thrown in one of the rules is displayed:Running
yarn run-tool .
: