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

Update: support multiple fixes in a report (fixes #7348) #8101

Merged
merged 9 commits into from Jun 18, 2017
Merged

Update: support multiple fixes in a report (fixes #7348) #8101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
926b00a
Update: support multiple fixes in a report (refs #7348)
mysticatea Feb 28, 2017
7271a5e
Chore: refactor fix of arrow-body-style
mysticatea Feb 18, 2017
5dba973
Chore: add a test for semi
mysticatea Feb 16, 2017
cc5de10
Merge changes from master branch.
mysticatea May 4, 2017
c888f09
add docs
mysticatea Jun 16, 2017
3847673
fix for negative indices
mysticatea Jun 16, 2017
a4c3372
Merge remote-tracking branch 'origin/master' into multiple-fixes-per-…
mysticatea Jun 16, 2017
f85aee0
add docs
mysticatea Jun 16, 2017
11a9043
fix for overlapping of negative ranges
mysticatea Jun 17, 2017
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
8 changes: 4 additions & 4 deletions lib/rule-context.js
View file
Open in desktop
Expand Up @@ -92,18 +92,18 @@ function mergeFixes(fixes, sourceCode) {
const start = fixes[0].range[0];
const end = fixes[fixes.length - 1].range[1];
let text = "";
let lastPos = Math.max(0, start);
let lastPos = Number.MIN_SAFE_INTEGER;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to use -Infinity in case someone uses extremely small numbers in fix ranges for whatever reason, but I think it's probably fine -- files usually don't have 253 BOMs. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah 😄

I expect that integer variables are faster by compiler optimization.


for (const fix of fixes) {
assert(fix.range[0] >= lastPos || fix.range[0] < 0, "Fix objects must not be overlapped in a report.");
assert(fix.range[0] >= lastPos, "Fix objects must not be overlapped in a report.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Requiring the ranges to be disjoint could make fixer.keep a lot less useful, I think. In #8067 for the no-else-return/no-useless-return fix (#8026), I intentionally want to keep all of an outer node (the function containing the fix) while modifying something inside of it.

With this API, I guess what I'd do is compute fixStart and fixEnd and yield that fix, then yield keepRange(functionStart, fixStart) and keepRange(fixEnd, functionEnd). That isn't so bad, I suppose, but seems a lot more clunky than saying "preserve the function and replace this text within the function".

One possible solution, if we're ok with it being a little magical, is to allow fixes to be completely contained within other fixes and always use the most specific one. Or maybe the fixes could be applied in order in a reasonable way, with later ones taking precedence.

(Although I recognize that there's maybe some disagreement about whether keep should be used that way in my use case anyway.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I think it's rare case if we need locking the outer node.
In no-else-return case, it needs locking only the previous if block.
In no-useless-return case, it needs locking only after the return statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense, I can see how the fixes for no-else-return and no-useless-return would work in that case. (I'm still a little unsure about how to properly reason about the implementation of no-useless-return, but it certainly seems easy and correct to call keepRange from the end of the fix to the end of the function.)

I think locking the outer node can be nice because it's safer and makes the rule simpler, but I can see the argument that it's usually not necessary. And if the long-term plan is to allow a distinction between "fixing" and "locking" operations, then I think it shouldn't be hard to allow the two types to overlap, which would address my concerns there.


if (fix.range[0] >= 0) {
text += originalText.slice(lastPos, fix.range[0]);
text += originalText.slice(Math.max(0, start, lastPos), fix.range[0]);
}
text += fix.text;
lastPos = fix.range[1];
}
text += originalText.slice(lastPos, end);
text += originalText.slice(Math.max(0, start, lastPos), end);

return { range: [start, end], text };
}
Expand Down
26 changes: 26 additions & 0 deletions tests/lib/rule-context.js
View file
Open in desktop
Expand Up @@ -237,6 +237,32 @@ describe("RuleContext", () => {

mockESLint.verify();
});

it("should throw an assertion error if ranges are overlapped.", () => {
const mockESLint = sandbox.mock(eslint);

mockESLint.expects("getSourceCode")
.returns({ text: "var foo = 100;" });
mockESLint.expects("report")
.never();

assert.throws(() => {
ruleContext.report({
node: {},
loc: {},
message: "Message",
fix(fixer) {
return [
fixer.removeRange([-1, 0]),
fixer.removeRange([-1, 0])
];
}
});
}, "Fix objects must not be overlapped in a report.");

mockESLint.verify();
});

});
});

Expand Down