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

Don't escape plus not followed by a whitespace #57

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

ocavue
Copy link
Contributor

@ocavue ocavue commented Dec 12, 2022

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

This PR improves the escape unsafe rule for +. If a + is not followed by [\ \t\r\n], then it won't turn into a list item, thus we don't need to escape it.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Dec 12, 2022
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Dec 12, 2022
@ocavue ocavue marked this pull request as ready for review December 12, 2022 14:04
@ocavue
Copy link
Contributor Author

ocavue commented Dec 12, 2022

Some issues about the CI:

  1. There is a typing error. It seems that it's not caused by this PR. I added a @ts-expect-error.
  2. The type coverage rate from the npm run test is lower than 100%. However I could not find the uncovered code in mdast-util-to-markdown/coverage/lcov-report/index.html, which shows 100%.

image

@ocavue ocavue mentioned this pull request Dec 12, 2022
@wooorm
Copy link
Member

wooorm commented Dec 13, 2022

type-coverage is not coverage, it’s something else.

I’ve updated the internal types for zwitch and TS 4.9: https://github.com/syntax-tree/mdast-util-to-markdown/commits/main.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

You can try and improve the - case with {atBreak: true, character: '-', after: '(?:[ \t\r\n\-])'}, and the * case with {atBreak: true, character: '*', after: '(?:[ \t\r\n\*])'}

@ocavue
Copy link
Contributor Author

ocavue commented Dec 13, 2022

The CI is green after rebasing the latest main branch. Thanks for your helping.


If I improve the - case and * case by adding after to these two rules, I cannot pass the test should prevent breaking out of code (-). It seems that inline-code.js is reusing these unsafe patterns to transform EOL into whitespace. I'm not sure what's the best way to fix it. Should I create a separate pattern list for inline-code.js (instead of reusing existed atBreak: true patterns in unsafe.js)?

@wooorm wooorm merged commit 4cb437c into syntax-tree:main Dec 13, 2022
@github-actions

This comment has been minimized.

wooorm added a commit that referenced this pull request Dec 13, 2022
@wooorm
Copy link
Member

wooorm commented Dec 13, 2022

I'm not sure what's the best way to fix it

These test check for something that was being escaped, but did not strictly need to be escaped. See 2f3eeb7

@wooorm wooorm added the 💪 phase/solved Post is done label Dec 13, 2022
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Dec 13, 2022
@wooorm
Copy link
Member

wooorm commented Dec 13, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants