Store "unnumbered" class in DocBook role attribute

[lifeunleaded]

Markdown allows marking a heading as unnumbered, which is stored as a class token internally. This change will recognize this particular class token and append it to the role attribute, or create a role attribute with it if needed. This does not imply any processing in DocBook but is intended to let customized stylesheets identify these sections and act accordingly.

The enrichRole function is intentionally designed to be able to take more class tokens in the future by extending the cand list

fixes #1402

[tarleb]

Thank you!

I think the code in enrichRole could possibly be cleaned up a little. Checkout the functions in the Data.List module, esp. lookup and partition, those could help to keep the function succinct.

Edit: also perhaps maybeToList, but that depends on the chosen approach.

[jgm]

Is the use of role="unnumbered" standard (i.e., supported in any standard xslt stylesheets)? Or is it that there's no standard way to do this, and you just want to provide a way that it can be done?

[lifeunleaded]

@tarleb Thank you. I did look for an import of Data.List and couldn't find one, so I thought it would be too intrusive to add. It can certainly be made more readable with Data.List functions.

@jgm No, there is no standard method (that I know of). In the issue descriptions people mentioned "workarounds" just to be able to find these sections, so that's the intention of this: The XPath selector to identify the sections that are supposed to be unnumbered is deterministic, but the work to then act accordingly with section labelling has to be done in the stylesheets.

[jgm]

No problem to use Data.List.

[lifeunleaded]

@tarleb @jgm On second thought, this is a trivial filter to create since the class token already comes in from the MD reader, and we can put attributes pairs that create XML attributes. Maybe just write it and add to the filters repo and point to it? That would allow anyone to tailor which attribute and token to use, if they already have a stylesheet. My Lua is not stellar, but something like below would do it? That would remove the need to hardcode it in entirely.

local function contains(table, val)
   for i=1,#table do
      if table[i] == val then 
         return true
      end
   end
   return false
end

function Header (hdr)
    if contains(hdr.classes, 'unnumbered'
    then
        hdr.attributes = {{'role','unnumbered'}}
    end
    return hdr
end

[tarleb]

Pandoc already does quite a few slightly opinionated things in various writers, so I think that making the change in the writer would be fine.

Slightly shorter filter:

function Header (hdr)
  if hdr.classes:includes 'unnumbered' then
    hdr.attributes.role = 'unnumbered'
    return hdr
  end
end

[jgm]

I agree, I think it's fine to change this in the writer.

[lifeunleaded]

Thank you. I will rewrite the enrichRole function and update.

[lifeunleaded]

@tarleb It is now more succinct. The Prelude lookup sufficed for this. To be honest I'm not sure it's more readable. I'll gladly take further pointers on changing it.

[tarleb]

Nice, thanks! This is good to merge from my POV.

Your comments on readability made me wonder, and I ended up writing an (untested!!) version trying to strike a balance between readability and and conciseness. Not sure if I succeeded, but here we go:

enrichRole :: [(Text, Text)] -> [Text] -> [(Text, Text)]
enrichRole mattrs cls = [("role", T.unword roles) | not (null roles)] <> nonRole
  where
    (roleAttr, nonRole) = partition (\(key, _v) -> key == "role") mattrs
    roles = nub $ ["unnumbered" | "unnumbered" `elem` cls] <> map snd roleAttr

[lifeunleaded]

@tarleb Thank you for the suggestion! I'm fine either way, although having cand separate was something I hoped to keep, as I can envision there are more class tokens that would make sense to add as role tokens in DocBook, and adding to that list is (perhaps) less intimidating to future contributors. Let me know if you would prefer it changed, and if not, if there's more to do before merge (rebase on current master?)

[tarleb]

Agreed, keeping the list of "role classes" separate seems sensible.

I just noticed that some lines break the 80 chars limit, we generally want to keep lines below that length. Once that's fixed we can squash-merge everything into a single commit; no need for a rebase.

EDIT: We're less strict about this limit in the tests, those are fine.

[lifeunleaded]

@tarleb Sorry, didn't get to the 80char fix before it merged. Will keep it in mind in the future.

[tarleb]

No problem at all. Thanks again, looking forward to the next one!