-
Notifications
You must be signed in to change notification settings - Fork 63
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
[v2.0.0-rc.0] Broken support for extended fence language #166
Closed
simonbrunel opened this issue
Dec 9, 2020
· 2 comments
· Fixed by GoogleForCreators/web-stories-wp#6765
Closed
[v2.0.0-rc.0] Broken support for extended fence language #166
simonbrunel opened this issue
Dec 9, 2020
· 2 comments
· Fixed by GoogleForCreators/web-stories-wp#6765
Comments
btmills
added a commit
that referenced
this issue
Dec 16, 2020
This was working correctly in v1 but stopped working when v2 switched to the new processor API. The generated filename was `0.js more words are ignored`. The test case for this was insufficiently strict: it asserted that the code block was extracted, which was sufficient with hard-coded extensions in v1, but it didn't assert the generated name was `0.js` now that we're taking the extension from the info string. Per CommonMark 0.29 [1]: > The line with the opening code fence may optionally contain some text > following the code fence; this is trimmed of leading and trailing > whitespace and called the info string. I've strengthened the tests for generated filename extensions and added a new test to ensure that leading whitespace is trimmed correctly. [1]: https://spec.commonmark.org/0.29/#info-string
You were right! Thanks so much for the bug report. Fix is in #167. |
btmills
added a commit
that referenced
this issue
Dec 16, 2020
The previous script worked on macOS and Linux, but I neglected to consider Windows. This wouldn't affect users thankfully, only developers, so the blast radius would have been small had it not been caught. Now that we're testing on Windows, future bugs like this should be caught. I'm out of ideas for doing this entirely inside npm scripts, so I broke down and did it in JS.
Whoops. Thanks for reporting that as well. #168 should fix it. |
btmills
added a commit
that referenced
this issue
Dec 20, 2020
* Chore: Test on Windows and macOS * Fix: npm prepare script on Windows (refs #166) The previous script worked on macOS and Linux, but I neglected to consider Windows. This wouldn't affect users thankfully, only developers, so the blast radius would have been small had it not been caught. Now that we're testing on Windows, future bugs like this should be caught. I'm out of ideas for doing this entirely inside npm scripts, so I broke down and did it in JS. * Chore: Increase test timeout for Windows and macOS runners They occasionally exhibit extremely slow filesystem performance. * Add npm-prepare script JSDoc header
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a regression of the feature introduced by a5d0cce.
For example, using the same input as the unit test:
Running ESLint on the markdown above:
Should report 2 errors with the following config:
I haven't been able to run tests locally (failed to
npm install
this repo on Windows) but I think that the associated unit test doesn't guarantee that the returned block will be processed by ESLint. I guess the returnedblock.filename
is'0.js more words are ignored'
which doesn't pass the glob matching on this virtual block filename.The text was updated successfully, but these errors were encountered: