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

GFM: Strikethrough support #62

Merged
merged 3 commits into from
May 23, 2024

Conversation

nobodywasishere
Copy link
Collaborator

This pull request adds GitHub Flavored Markdown (GFM) strikethrough support (~~hello~~ => hello).

This also adds the gfm-spec.txt downloaded from here. To prevent a lot of broken specs that won't be fixed by this PR, the strikethrough specs have been copied to gfm-extra.txt temporarily.

require "./src/markd"

markdown = <<-MD
# Hello Markd

~~parser~~
:abc:

> Yet another markdown ~~parser :abc:~~ built for speed, written in Crystal, Compliant to CommonMark specification.
MD

options = Markd::Options.new(gfm: true, emoji: true)
puts Markd.to_html(markdown, options)
<h1>Hello Markd</h1>
<p><del>parser</del>
🔤</p>
<blockquote>
<p>Yet another markdown <del>parser 🔤</del> built for speed, written in Crystal, Compliant to CommonMark specification.</p>
</blockquote>

gfm-extra.txt contains edge cases not covered by gfm-spec.txt,
and is a temporary staging ground as gfm functionality is implemented
@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label May 21, 2024
Copy link

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

This is great. I left one nitpicky comment. Thank you for your strategic approach to this.

# NOTE(margret): Temporarily copying strikethrough specs
# describe_spec("fixtures/gfm-spec.txt", gfm: true)

describe_spec("fixtures/gfm-extra.txt", gfm: true)

Choose a reason for hiding this comment

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

I appreciate your strategic approach here, thank you.

My only suggestion is that this file be named something other than "gfm-extra". How about "working-gfm-spec.txt" or something similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would work for me. I also wanted a file to add GFM specs for stuff that wasn't covered by the normal spec (like leaving ~ if there's only one set around a char). I can split these into two different files as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I think it'd be better to leave gfm-spec.txt as-is and instead of bringing pieces over to a new file piecemeal, to mark things in gfm-spec that don't/shouldn't pass as pending. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Went with the latter as there are currently only ~36 failing tests in gfm-spec.txt

Copy link
Owner

@icyleaf icyleaf left a comment

Choose a reason for hiding this comment

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

LGTM

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 23, 2024
@icyleaf icyleaf merged commit 16e1abc into icyleaf:master May 23, 2024
2 of 4 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged labels May 23, 2024
@nobodywasishere nobodywasishere deleted the nobody/gfm-strikethrough branch May 23, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants