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

Respect EOL character from .editorconfig #590

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Conversation

CloudNStoyan
Copy link
Contributor

Fixes #527

It uses the editorconfig npm package https://www.npmjs.com/package/editorconfig (which has 4+ million weekly downloads as of today) to parse the .editorconfig.

I didn't know where to put the function so the lib/string.ts seemed most appropriate.

Sadly I couldn't find a way to test it with a real .editorconfig file, so the tests only test if it fallbacks to EOL from node:os (If anybody is willing to give me a hint as to how to write the tests I will write them).

@bmish bmish added the bug Something isn't working label Dec 10, 2024
@CloudNStoyan CloudNStoyan requested a review from bmish December 11, 2024 09:41
@CloudNStoyan
Copy link
Contributor Author

Using the mockFs worked the tests are now back at 100% coverage, thanks :)

import mockFs from 'mock-fs';
import { jest } from '@jest/globals';

describe('string (getEndOfLine)', function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a test in here with an empty .editorconfig and also a non-existent .editorconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, do you want me to remove the test from string-test.ts named handles when .editorconfig is not available and fallbacks to 'EOL' from 'node:os' as it will overlap with the non-existent .editorconfig test?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's fine to have both since one is a unit test and one is an integration test.

// This should be the correct path to resolve the `.editorconfig` for these
// specific markdown files.
const config = editorconfig.parseSync('./docs/rules/markdown.md');
const config = editorconfig.parseSync('markdown.md');
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add a comment about how we're just using a hypothetical markdown file in the plugin root to check for any markdown config?

import mockFs from 'mock-fs';
import { jest } from '@jest/globals';

describe('string (getEndOfLine)', function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add at least one full test in here (copy from test/lib/generate/sorting-test.ts) that actually runs generate() and verifies that all the snapshots match? That way, we can test that getEndOfLine is actually being used everywhere and the snapshots end up with the correct character.

@CloudNStoyan
Copy link
Contributor Author

CloudNStoyan commented Dec 12, 2024

I had to move some of the getEndOfLine invocations inside their respective methods because the mock-fs couldn't mock them because the function was invoked when the generator function is imported instead of when it was ran.

Given that the README.md mock has newline in it, it works great to test if the rest of the code is using the same newline but as far as I can tell the Jest snapshot doesn't respect our newline choices.

Edit: I did find out that if we JSON.stringify the output to escape the new lines it will show them correctly inside the snapshot if that is a thing you would like

// before
expect(readFileSync('README.md', 'utf8')).toMatchSnapshot();

// after
expect(
        JSON.stringify(readFileSync('README.md', 'utf8')),
      ).toMatchSnapshot();

@CloudNStoyan CloudNStoyan requested a review from bmish December 12, 2024 00:08
@bmish
Copy link
Owner

bmish commented Dec 12, 2024

No need to use JSON.stringify. As long as the snapshot test will fail if the wrong EOL character is used, then I'm happy with it.

* main:
  Switch to `change-case` (bmish#591)
  chore(deps-dev): Bump typescript-eslint from 8.17.0 to 8.18.0 (bmish#589)
@bmish bmish changed the title Respect .editorconfig's end_of_line setting and fallbacks to EOL from node:os Respect EOL character from .editorconfig Dec 12, 2024
Copy link
Owner

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Been wanting to get this bug fixed for a long time.

@bmish bmish merged commit cd94000 into bmish:main Dec 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect existing newline character choice
2 participants