Respect EOL character from .editorconfig
#590
Conversation
|
Using the |
| import mockFs from 'mock-fs'; | ||
| import { jest } from '@jest/globals'; | ||
|
|
||
| describe('string (getEndOfLine)', function () { |
There was a problem hiding this comment.
Can you add a test in here with an empty .editorconfig and also a non-existent .editorconfig?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 () { |
There was a problem hiding this comment.
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.
|
I had to move some of the Given that the 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(); |
|
No need to use |
.editorconfig
bmish
left a comment
There was a problem hiding this comment.
Thanks a lot! Been wanting to get this bug fixed for a long time.
Fixes #527
It uses the
editorconfignpm 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.tsseemed most appropriate.Sadly I couldn't find a way to test it with a real
.editorconfigfile, so the tests only test if it fallbacks toEOLfromnode:os(If anybody is willing to give me a hint as to how to write the tests I will write them).