mity / md4c

C Markdown parser. Fast. SAX-like interface. Compliant to CommonMark specification.
MIT License
776 stars 146 forks source link

Implement task lists. #50

Closed mity closed 5 years ago

mity commented 5 years ago

Implementation of GitHub-style task lists.

Fixes #30.

To do:

codecov[bot] commented 5 years ago

Codecov Report

Merging #50 into master will increase coverage by 0.02%. The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   92.62%   92.64%   +0.02%     
==========================================
  Files           1        1              
  Lines        2548     2570      +22     
==========================================
+ Hits         2360     2381      +21     
- Misses        188      189       +1
Impacted Files Coverage Δ
md4c/md4c.c 92.64% <96.42%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 03f5868...1e8b588. Read the comment docs.

codecov[bot] commented 5 years ago

Codecov Report

Merging #50 into master will increase coverage by 0.03%. The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   92.62%   92.65%   +0.03%     
==========================================
  Files           1        1              
  Lines        2548     2573      +25     
==========================================
+ Hits         2360     2384      +24     
- Misses        188      189       +1
Impacted Files Coverage Δ
md4c/md4c.c 92.65% <96.77%> (+0.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 03f5868...d9da037. Read the comment docs.

ec1oud commented 5 years ago

I tried but it's not working yet... I must be missing something. is_task is false and mark is empty.

https://codereview.qt-project.org/#/c/252383/

Why is it that MD_BLOCK_UL_DETAIL has a mark and MD_BLOCK_LI_DETAIL has task_mark? Wouldn't it be more flexible to have mark in MD_BLOCK_LI_DETAIL only, in case someone mixes marks within the same list? Or is that not allowed?

mity commented 5 years ago

I tried but it's not working yet... I must be missing something. is_task is false and mark is empty.

It already passes all the tests in the tests/tasklists.txt so md2html gets the detailed data correctly. Did you use MD_FLAG_TASKLISTS? The feature is an extension beyond the standard CommonMark specification.

Why is it that MD_BLOCK_UL_DETAIL has a mark and MD_BLOCK_LI_DETAIL has task_mark? Wouldn't it be more flexible to have mark in MD_BLOCK_LI_DETAIL only, in case someone mixes marks within the same list? Or is that not allowed?

Unordered list must use have the same mark (for example * or -) for all the items or it is not considered one list. Similarly, ordered lists must use the same char after the number (. or )) for all items. So, it really is property of complete list, not of every list item.

Also this was part of the API before working on this feature.

Duplicating the list detail data in the item detail is potentially possible, but we would have to distinguish ordered list item and unordered list items, and I am not sure whether it is worth of the complication. (Yes, if you haven't notice, ordered list can also have task items. I follow cmark-gfm here.)

The task marks are different in that it is really property of every item. The task items and ordinary items may be mixed in a single list.

ec1oud commented 5 years ago

Yeah I forgot that the feature flags are in more than one place in my patches... I'd better try to come up with a way to define them in only one place. So it works after fixing that.

Do you think that MD_FLAG_TASKLISTS and such will be there for the long term? I could remove those from the Qt API and just offer the two dialects, and allow the meaning to change over time. But it will mean Qt users can't disable features within one dialect. OTOH keeping all those feature flags means when you add one, Qt should add it too; and if you ever remove one, we have to deprecate it in Qt API (because it will no longer do anything), but we can't remove it.

Also, most are positive (MD_FLAG_PERMISSIVEWWWAUTOLINKS) and some are negative (MD_FLAG_NOINDENTEDCODEBLOCKS). Are you sure you want to keep them that way?

mity commented 5 years ago

I could remove those from the Qt API and just offer the two dialects, and allow the meaning to change over time.

IDK what makes best sense on Qt API level.

But consider this scenario:

  1. GitHub tomorrow adds new syntax feature called XY.
  2. MD4C eventually implements it, adds new MD_FLAG_XY and changes MD_DIALECT_GITHUB to include it.
  3. New rebuilds of Qt (if Qt uses the macro MD_DIALECT_GITHUB) could now get new block or span types, it is not ready for.

IMHO the safest way from compatibility point of view is, if Qt calls MD4C with the manual or'ing of individual flags directly, no matter what Qt offers to apps in its API.

Also, most are positive (MD_FLAG_PERMISSIVEWWWAUTOLINKS) and some are negative (MD_FLAG_NOINDENTEDCODEBLOCKS). Are you sure you want to keep them that way?

The rule so far is flags == 0 means CommonMark specification. That core is safest to use.

The negatives are for things some people see as misfeatures in the specification, and there is some demand/reasoning in order to allow suppressing it. Take MD_FLAG_NOHTML as an example. Markdown syntax originally began as a more comfortable way how to write HTML. But if you use it in any other context, the raw HTML blocks/inlines often do not make much sense.

Similarly MD_FLAG_NOINDENTEDCODEBLOCKS is seen by some people as bad idea given the complicated indentation rules in e.g. nested lists; and there is fine alternative in the fenced code blocks.

The extensions (i.e. the positive flags) are more likely to change to bring them to the original behavior (e.g. github markdown) if some differences are observed. The situation is that the extensions typically have no or very poor specification; or that there are bigger differences among various Markdown implementations. There is sort of Babel situation, unfortunately.

Do you think that MD_FLAG_TASKLISTS and such will be there for the long term?

In short, yes, I think the flags will stick with us for a long time.

I hope that at least the most popular extensions (tables, maybe task lists, permissive auto-links) may eventually end up in the CommonMark specification. But given the slow progress on it so far, it may take years. (No such change happened since I started the work on MD4C about 3 years ago. In this period there were only changes how to interpret some corner cases and clarifications in spec wording.)

If/when that happens, it shall likely deserve bump of the major version; or to require MD_PARSER::abi_version=1 to identify that the caller is ready to get broader sets of block and/or span types into the callbacks. In either case we can keep the flag macro there (it would just become noop).

mity commented 5 years ago

@ec1oud Are you satisfied with my previous answers? As this thread affects the abi maintenance policy, I would like to be sure we have reached a consensus on this before making any release with the shared lib support.

Also, if you are ok with the interface for the task lists, I'd merge it before the release. If not, it can be left for some future release.

ec1oud commented 5 years ago

I think it's all good, I'm just trying to get checklists actually working. Getting close though.