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
Chore: refactor reporting logic (refs #9161) #9168
Conversation
LGTM |
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @platinumazure and @mysticatea to be potential reviewers. |
e44f4a9
to
015d843
Compare
LGTM |
LGTM |
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.
Left a few comments where I couldn't quite follow what I was going on. At first glance they seem like mistakes, but I might be missing something.
loc: location, | ||
message, | ||
*fix() { | ||
yield { range: [1, 2], text: "foo" }; |
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.
Confused about this test. Seems like it might be using the same setup as the previous test, but the title of this test implies there should only be one fix.
tests/lib/report-translator.js
Outdated
const reportDescriptor = { | ||
node, | ||
message, | ||
fix: () => [{ range: [-1, 3], text: "\uFEFFfoo" }, { range: [4, 5], text: "x" }] |
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.
Confused about this test. The title mentions removing the BOM, but I see the BOM being preserved (I think).
LGTM |
63713a1
to
f516143
Compare
LGTM |
LGTM |
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.
Thanks for fixing and for doing this awesome refactor. LGTM!
While investigating the performance impact of this change, I tried modifying the `createReportTranslator` API, but I forgot to change the JSDoc comment back after reverting that modification.
LGTM |
Thanks for reviewing! |
What is the purpose of this pull request? (put an "X" next to item)
[x] Other, please explain:
What changes did you make? (Give an overview)
This refactors some of the logic around
RuleContext
, as described in #9161.Linter
instance, and the rule ID and severity are added to the report object from a closure. This avoids a circular dependency between modules. It also prevents rules from impersonating or tampering with other rules by using something likecontext._linter.report("some-other-rule", 2, node, "some message")
.context.report
arguments toLinter
problem descriptors now happens in one place (report-translator
). Previously, the logic was spread out betweenRuleContext
and various places inLinter
, which made it confusing to follow and has caused some bugs in the past. That logic has been refactored now and should be easier to follow.Linter#report
is no longer accessible from the public API, because it no longer exists.Is there anything you'd like reviewers to focus on?
Should this be considered a breaking change? I'm pretty sure it doesn't break any documented APIs, but it will break anyone who was relying on undocumented behavior, e.g. calling
Linter#report
directly for some reason. We have a very explicit warning that only the documented APIs are public, so I personally don't think this is breaking.