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.

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

fix: Account for BOM in the processor #282

Merged
merged 2 commits into from
Sep 16, 2024
Merged

fix: Account for BOM in the processor #282

merged 2 commits into from
Sep 16, 2024

Conversation

mdjermanovic
Copy link
Member

The processor doesn't account for BOM, which would be passed in by ESLint if the file has it.

This causes incorrect adjustments of locations and fix ranges.

Repro: https://github.com/mdjermanovic/eslint-markdown-bom

In the above repro, space-in-parens rule fixes ( a) to ( ) instead of (a).

This PR updates preprocess() to remove BOM from the text it operates on.

Comment on lines 838 to 844
it("should translate indented column numbers", () => {
const result = processor.postprocess(messages);

assert.strictEqual(result[2].column, 9);
assert.strictEqual(result[3].column, 4);
assert.strictEqual(result[4].column, 2);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only test that was failing with BOM before the code change.

Comment on lines 846 to 851
it("should adjust fix range properties", () => {
const result = processor.postprocess(messages);

assert(result[2].fix.range, [185, 185]);
assert(result[4].fix.range, [264, 265]);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how, but this test was passing with BOM before the code change.

Copy link
Member Author

@mdjermanovic mdjermanovic Sep 13, 2024

Choose a reason for hiding this comment

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

Oh, this test was missing .deepStrictEqual after assert.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, in 68aede7 I fixed this test and added another one as per the repro from the original post. Both would fail without the code change.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nzakas nzakas merged commit 01bceae into main Sep 16, 2024
13 checks passed
@nzakas nzakas deleted the fix-bom branch September 16, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants