Convert tidier.js to TypeScript && test coverage of tidier.ts#3899
Convert tidier.js to TypeScript && test coverage of tidier.ts#3899kashish00208 wants to merge 5 commits intoprocessing:develop-codemirror-v6from
Conversation
raclim
left a comment
There was a problem hiding this comment.
Thanks so much for your work! This looks great so far!
We recently merged in a PR that added tests for tidier.js as a JavaScript file—would you be open to updating this PR with the changes from the develop-codemirror-v6 branch, convert that test to TypeScript, and also add in any additional updates to the test that you think would be beneficial?
a8040cf to
67c3954
Compare
I've updated the PR! I merged the latest changes from develop-codemirror-v6, deleted the JS test file, and fully migrated the suite to TypeScript. I also added the beneficial tests for empty documents and cursor stability do let me know if any chnages needed . |
|
Thanks so much for the updates! I think these feel overall good to me—also tagging @khanniie for review! |
khanniie
left a comment
There was a problem hiding this comment.
Overall looking super solid! Thank you so much, this is great! Just a few tiny comments
| import type { Plugin } from 'prettier'; | ||
|
|
||
| function prettierFormatWithCursor(parser, plugins, cmView) { | ||
| type parserTypes = 'babel' | 'html' | 'css'; |
There was a problem hiding this comment.
Small nit but I'm not sure I understand why one type is capitalized and the other isn't! For consistency with the style guide maybe we can call this one ParserTypes?
| tidyCodeWithPrettier(cmView, 'javascript'); | ||
|
|
||
| const formattedJs = getDocText(cmView); | ||
| expect(formattedJs).toContain('console.log("hi");'); |
There was a problem hiding this comment.
for this test, can we can test for the fully properly formatted JavaScript? I don't think this exactly tests for the correctness of the formatter. You should be able to include new lines (\n) in the test string
| tidyCodeWithPrettier(cmView, 'html'); | ||
|
|
||
| const formattedHtml = getDocText(cmView); | ||
| expect(formattedHtml).toContain('<p>hello</p>'); |
There was a problem hiding this comment.
Same here can we test for the fully properly formatted output?
| tidyCodeWithPrettier(cmView, 'css'); | ||
|
|
||
| const formattedCss = getDocText(cmView); | ||
| expect(formattedCss).toContain('margin: 0;'); |
|
@khanniie thank you pointing it out I've updated the tests to be more rigorous. Instead of using .toContain(), I switched to .toBe() to verify the exact output from Prettier . |
Fixes #3815 #3816
Changes:
This PR implements the following changes based on issue #3816 #3815:
I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123