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(extensions): Add support for task list extension #16

Merged
merged 2 commits into from
Nov 15, 2019

Conversation

gillchristian
Copy link

Solves #15

Added support for the task list extension.

As I mentioned in the issue I (kinda) don't know what I'm doing. But the test case passed so 🤷‍♂

I have two questions about my PR:

  • I see that test cases are quite simple, I added one using the example from the spec (https://github.github.com/gfm/#task-list-items-extension-). Would that be the right test?
  • I did not manage to run the benchmakrs (will try again later). Anyways, I added cases of task lists to bench/full-sample.md. Is that find? Do I have to do something about the benchmarks?

@kivikakk
Copy link
Owner

This looks perfect, honestly; thank you!

  • The test looks fine — we don't need to be extensive, just make sure that the options are being passed through to the underlying library correctly. This does that ✅
  • I don't know about the benchmarks, honestly — I've left them unchanged from the upstream cmark-hs. Adding the extensions does make sense, so this is good to merge ✅

Thank you so much!

@kivikakk kivikakk merged commit 3bd7966 into kivikakk:master Nov 15, 2019
@kivikakk
Copy link
Owner

Let me know if you'd like a version bump/new release on Hackage.

@gillchristian
Copy link
Author

Wow already merged 😮 Thanks for the fast response 🎉

Yeah, the version bump would be great. But on the meantime I can install from GitHub, already tried that to double check if it was working.

@kivikakk
Copy link
Owner

Thanks for making it so easy to merge! 🚀

0.2.1 is tagged and published on Hackage 👍

@gillchristian gillchristian mentioned this pull request Nov 15, 2019
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