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

Sample lua custom writer fixes (mostly editorial) #7487

Closed
wlupton opened this issue Aug 10, 2021 · 9 comments
Closed

Sample lua custom writer fixes (mostly editorial) #7487

wlupton opened this issue Aug 10, 2021 · 9 comments

Comments

@wlupton
Copy link
Contributor

wlupton commented Aug 10, 2021

Here are some comments on the v2.14.1 sample lua custom writer. Please let me know if I should create separate issues for any of them.

  1. Attribute sorting: attributes(attr) uses pairs(attr), which I believe means that the attribute order is unpredictable (not a problem per se, but annoying if ever comparing the output from multiple runs). Probably it should do whatever the Haskell HTML writer does?

  2. Link attributes: Link(s, tgt, tit, attr) ignores the attr argument.

  3. Image versus figure: CaptionedImage(src, tit, caption, attr) doesn't check for empty captions. I'm not sure about this one: I did hit it in real life, but it's possible that it was caused by a lua filter fiddling with the caption.

  4. Unintentional head global? Table(caption, aligns, widths, headers, rows) can update a global head variable that doesn't appear to be declared or used anywhere. Probably it can just be removed?

@wlupton wlupton added the bug label Aug 10, 2021
@jgm
Copy link
Owner

jgm commented Aug 10, 2021

PRs would be welcome for any of these!

@wlupton
Copy link
Contributor Author

wlupton commented Aug 10, 2021

Of course :).

What would be the preferred resolution for item 1 (attribute ordering)? Could you possibly briefly summarize what the Haskell HTML writer does?

@tarleb
Copy link
Collaborator

tarleb commented Aug 11, 2021 via email

@wlupton
Copy link
Contributor Author

wlupton commented Aug 11, 2021

Thanks. I'll create a separate issue (#7489).

wlupton added a commit to BroadbandForum/pandoc that referenced this issue Aug 11, 2021
These address most of the items mentioned in jgm#7487.
There's also a table caption fix (the caption wasn't escaped).
@wlupton
Copy link
Contributor Author

wlupton commented Aug 11, 2021

PR #7493 addresses items 2-4 plus fixes another small problem that I noticed when making the changes (the table caption wasn't escaped).

Comparing the built-in HTML writer with the updated sample.lua I noticed:

  • The writer needs to handle attribute data- prefixes, so it has to know the standard attributes for each element. This could be hard-coded in the lua or could a utility function supply this info?
  • The built-in writer is outputting some aria- attributes (these relate to accessibility?)
  • sample.lua uses a mixture of single and double quotes. Not a problem, but mildly annoying! Personally I prefer single quotes in code, which makes it easier to use double quotes in output: would this be acceptable?
  • sample.lua doesn't usually put a space before /> following an attribute value, whereas the built-in writer usually (always?) does. Again not a problem, but consistency would be nice? (Personally I prefer not to output such spaces.)
  • sample.lua is a little more liberal with newlines. This really isn't a problem!

I mention all the above just to solicit some thoughts on whether it's worth trying to make sample.lua generate the same output as the built-in HTML writer (maybe ignoring unquoted whitespace). If it did then this could be validated on some test documents, which might help to keep it up to date.

wlupton added a commit to BroadbandForum/pandoc that referenced this issue Aug 11, 2021
These address most of the items mentioned in jgm#7487.
There's also a table caption fix (the caption wasn't escaped).
jgm pushed a commit that referenced this issue Aug 12, 2021
These address most of the items mentioned in #7487.
There's also a table caption fix (the caption wasn't escaped).
@wlupton
Copy link
Contributor Author

wlupton commented Aug 13, 2021

PR #7493 notes the single/double quote inconsistency and I've offered to push some more updates. Note that there are some related comments (some of which overlap) in #6729 and I'll take those into account too.

Something else to check is that everything is escaped where necessary. I fixed one such case (table captions) but I suspect that there may be more.

@mb21
Copy link
Collaborator

mb21 commented Nov 19, 2021

Should we close this issue in favour of more specific ones or new PRs?

@wlupton
Copy link
Contributor Author

wlupton commented Nov 19, 2021

I can take an action to capture any outstanding items in new issues, potentially with PRs, at which point we can close this.

@tarleb
Copy link
Collaborator

tarleb commented Jan 31, 2022

I've update sample.lua to use single quotes for strings, which I believe was the last point without a separate issue. Personally I'd consider aria attributes out of scope for the custom writer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants