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

feat: improve support of backtick strings #95

Merged
merged 9 commits into from
May 6, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 4, 2021

Alternate to #94

Only supports code span delimited with a single backtick char and gives up if it finds an edge case.

List of unsupported markdown-compliant edge cases I could think of:

test: string ``code `span` with backticks inside`` not supported
test: string `code span ending with a backslash\\` not supported
test: string containing escaped \` `code span` not supported
`test`: message starting with a backslash not supported

I wasn't sure how to add tests for that, but it works from what I have tested locally.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

looks good, I think.
What you could do for tests is add some trivial commits to this PR and test against the commits that this comprises. See how the tests currently do it.
I'm going to suggest that the next release will be a semver-minor, so you could bake that into your test case with a --start-ref and --end-ref and add commits to this PR with just a change of whitespace in this file, or the README, that have commit messages that form part of the test suite. That's perfectly OK to do here. If we have any awkward breakage during the next release we can sort that out later.
Have a quick go, if it gets too complicated then no big deal I think but it would be nice to see how this works and bake it into the tests.

@aduh95 aduh95 requested a review from rvagg May 5, 2021 10:10
@rvagg
Copy link
Member

rvagg commented May 6, 2021

looks good I think but the test text fails lint with unnecessary escapes

@aduh95 aduh95 force-pushed the cleaner-markdown branch from 6a98e28 to fb91ea6 Compare May 6, 2021 07:51
@aduh95 aduh95 force-pushed the cleaner-markdown branch 2 times, most recently from 0ee4588 to fd7162f Compare May 6, 2021 08:26
@aduh95 aduh95 force-pushed the cleaner-markdown branch from fd7162f to deb28cc Compare May 6, 2021 08:45
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

sweet

@rvagg rvagg merged commit 575a5e2 into nodejs:main May 6, 2021
@rvagg
Copy link
Member

rvagg commented May 6, 2021

ok, github didn't like the empty commits, I had to reconstruct that all locally but it should be publisahed now as 2.6.0

@aduh95 aduh95 deleted the cleaner-markdown branch May 6, 2021 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants