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

Support for <indexterm>s when reading DocBook #7607

Merged
merged 4 commits into from
Nov 5, 2021
Merged

Conversation

bigsmoke
Copy link
Contributor

@bigsmoke bigsmoke commented Oct 5, 2021

This PR resolves #7427. This was my first Haskell patch ever, so I'm sure that my additions, although functional, are an eyesore to seasoned Haskell coders. Please feel even more free than you usually would to correct/criticize my code.

indextermNaryTextAsAttr :: Text -> Element -> Maybe (Text, Text)
indextermNaryTextAsAttr n e = case findChild q e of
Nothing -> Nothing
Just naryEl -> Just (n, (strContent naryEl))
Copy link
Owner

Choose a reason for hiding this comment

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

Here it's idiomatic to omit the parentheses around (strContent naryEl).

@jgm
Copy link
Owner

jgm commented Oct 5, 2021

Should we include the pagenum attribute?

What about see and seealso elements (there can be multiple of the latter)?

@jgm
Copy link
Owner

jgm commented Oct 5, 2021

A test would be nice, too!
Easiest would be a "command test", since there isn't currently a Tests.Reader.DocBook module.

@bigsmoke
Copy link
Contributor Author

bigsmoke commented Oct 6, 2021

Should we include the pagenum attribute?

I don't think so. That is only used within <index> sections, which, like we discussed in #7608, makes most sense to continue to ignore in Pandoc.

What about see and seealso elements (there can be multiple of the latter)?

I will follow up with a new PR request with (partial) support for <see> and <seealso> elements. They will be more straight-forward and less flaky to express as attributes to the span than <primar>, <secondary> and <tertiary>, because the latter actually have a mixed content model (which AsciiDoc doesn't have, so that it doesn't become a problem for me personally)..

A test would be nice, too!
Easiest would be a "command test", since there isn't currently a Tests.Reader.DocBook module.

I will included a test too. Thank you for the pointer! ☺

@bigsmoke
Copy link
Contributor Author

I have improved the patch. You stimulated me to go a bit further and also do the Right Thing™ with mixed-content child element of <indexterm>, including a test case to ensure that the existing strContentRecursive that I reused indeed did what I hoped it would: to flatten the mixed content of, for example, a <primary> element into a string suitable for use in an attribute.

Thanks for sticking with me, @jgm

@bigsmoke
Copy link
Contributor Author

bigsmoke commented Nov 5, 2021

I reworded one of my commit messages and did a git push --force-with-lease, because one of the GitHub sanity check choked on its length 5 days ago.

@jgm jgm merged commit 7a70a46 into jgm:master Nov 5, 2021
@jgm
Copy link
Owner

jgm commented Nov 5, 2021

Great, thanks.

@bigsmoke bigsmoke deleted the issue7427 branch November 9, 2021 16:17
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.

<indexterm> support in DocBook reader?
2 participants