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

asciidoc writer, checklists fix #7832

Merged
merged 4 commits into from
Jan 16, 2022
Merged

Conversation

ricnorr
Copy link
Contributor

@ricnorr ricnorr commented Jan 14, 2022

Requesting a code review. See issue #7798

@jgm
Copy link
Owner

jgm commented Jan 14, 2022

In general this looks very good.

Some questions: I checked the asciidoc documentation for task lists, and found this:
https://docs.asciidoctor.org/asciidoc/latest/lists/checklist/
This suggests that

  1. task lists are a built-in feature of asciidoc, not an extension that would make sense to disable, and
  2. they only work for bullet lists, not ordered lists.

I might be wrong in one of these impressions -- in which case correct me.
But if these things are right, then I think the code for task lists should be removed from the clause for OrderedList. And either we should make task_lists part of the default extensions for asciidoc (not just "all extensions"), or we should make this code unconditional so that it is not sensitive to an extension. (Either of these would also avoid something I thought was awkward in the test suite, using "all extensions" as if it is a default in defopts.)

@ricnorr
Copy link
Contributor Author

ricnorr commented Jan 14, 2022

In general this looks very good.

Some questions: I checked the asciidoc documentation for task lists, and found this: https://docs.asciidoctor.org/asciidoc/latest/lists/checklist/ This suggests that

  1. task lists are a built-in feature of asciidoc, not an extension that would make sense to disable, and
  2. they only work for bullet lists, not ordered lists.

I might be wrong in one of these impressions -- in which case correct me. But if these things are right, then I think the code for task lists should be removed from the clause for OrderedList. And either we should make task_lists part of the default extensions for asciidoc (not just "all extensions"), or we should make this code unconditional so that it is not sensitive to an extension. (Either of these would also avoid something I thought was awkward in the test suite, using "all extensions" as if it is a default in defopts.)

Yes, you are right, review fix please

@@ -434,6 +434,9 @@ getDefaultExtensions "jats_articleauthoring" = getDefaultExtensions "jats"
getDefaultExtensions "opml" = pandocExtensions -- affects notes
getDefaultExtensions "markua" = extensionsFromList
[]
getDefaultExtensions "asciidoc" = extensionsFromList
Copy link
Owner

Choose a reason for hiding this comment

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

This should be formatted like the others, so the = lines up.

Comment on lines 93 to 97
, test (asciidocWithOpts def) "bullet without task_lists" $
bulletList [plain "☐ a", plain "☒ b"] =?> unlines
[ "* ☐ a"
, "* ☒ b"
]
Copy link
Owner

Choose a reason for hiding this comment

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

If we leave task_lists as an extension that can be disabled, then it makes sense to test what happens when it is disabled. But I'm still not sure why we should make task list rendering something you can disable with an extension.

So, I'd favor

  • removing task_list from the available extensions for asciidoc.
  • changing the code to do the task list transformation if asciidoctorVariant is set in WriterState, otherwise not. (A cursory glance through the original asciidoc user guide https://asciidoc-py.github.io/userguide.html doesn't show anything about checklists, so it seems to be an asciidoctor-only feature.)

With these changes, task list syntax will be used automatically with -t asciidoctor but not with -t asciidoc, which I think is what we want. Extensions should be used only when it makes sense to disable them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, review please

Comment on lines 672 to 677
handleTaskListItemNoExtsCheck :: ([Inline] -> [Inline]) -> [Block] -> [Block]
handleTaskListItemNoExtsCheck handleInlines bls =
handleItem bls
where
handleItem (Plain is : bs) = Plain (handleInlines is) : bs
handleItem (Para is : bs) = Para (handleInlines is) : bs
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to avoid the API change this would require.
Why not just call handleTaskListItem, and add Ext_task_lists to the extensions parameter when calling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ricnorr ricnorr force-pushed the asciidoc-writer-checklists branch from 341abf8 to 41d131e Compare January 16, 2022 11:50
@jgm jgm merged commit b683b8d into jgm:master Jan 16, 2022
@jgm
Copy link
Owner

jgm commented Jan 16, 2022

Great, thanks!

@tarleb tarleb mentioned this pull request Jan 29, 2022
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