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

markup/goldmark: Support passthrough extension #11866

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

j2kun
Copy link
Contributor

@j2kun j2kun commented Jan 6, 2024

Fixes #10894

Creating as a draft so that it can be discussed in the issue, and while I work on adding additional tests. Implementation by @janhuenermann, tests by me.

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2024

CLA assistant check
All committers have signed the CLA.

@j2kun
Copy link
Contributor Author

j2kun commented Jan 21, 2024

Trying the initial integration with my blog, I'm seeing some issues with the renderer. When I configure my personal block via https://github.com/j2kun/math-intersect-programming/commit/892f42db29a388ae18e7aab70f6b3d4a579b66a4:

[markup.goldmark.extensions.passthrough]
  enable = true
  blockDelimiters = [
    {open = "$$", close = "$$"},
    {open = "\\[", close = "\\]"},
  ]
  inlineDelimiters = [
    {open = "$", close = "$"},
    {open = "\\(", close = "$\\)"},
  ]

I see

ERROR render of "page" failed: "/home/j2kun/blog/themes/paperesque/layouts/_default/baseof.html:9:7": execute of template failed: template: _default/single.html:9:7: executing "_default/single.html" at <partial "meta.html" .>: error calling partial: execute of template failed: template: _internal/opengraph.html:2:98: executing "_internal/opengraph.html" at <.Summary>: error calling Summary: runtime error: invalid memory address or nil pointer dereference
Total in 206 ms
Error: error building site: render: failed to render pages: render of "page" failed: "/home/j2kun/blog/themes/paperesque/layouts/_default/baseof.html:9:7": execute of template failed: template: _default/single.html:9:7: executing "_default/single.html" at <partial "meta.html" .>: error calling partial: execute of template failed: template: _internal/opengraph.html:2:98: executing "_internal/opengraph.html" at <.Summary>: error calling Summary: runtime error: invalid memory address or nil pointer dereference

Based on the opengraph.html, this seems related to hugo, not the theme I'm using: https://github.com/gohugoio/hugo/blob/a541e3b4d48cac704185a644539d3501ff183b19/tpl/tplimpl/embedded/templates/opengraph.html

But I'm not sure how to narrow this down further. Could be an issue with my building hugo at head (a541e3b) + this PR

Note this only occurs due to the inclusion of the {open = "\\[", close = "\\]"}, line. Removing that and the error does not occur.

@j2kun j2kun marked this pull request as ready for review January 21, 2024 06:24
@jmooring
Copy link
Member

jmooring commented Jan 21, 2024

I may have some time to look at this tomorrow, but the config structure is different that what we (I) discussed here:
gohugoio/hugo-goldmark-extensions#2 (comment)

Of the two options, I like the second one better.

Whether a delim is an opener or a closer should be inferred from its position in the array. There's an example of this in the config structure for tdewolff/minify, e.g.,

[minify.tdewolff.html]
    templateDelims = ['<?php', '?>']

I took what you have done so far for quick test drive with the test cases repo/branch:

git clone --single-branch -b hugo-github-issue-10894 https://github.com/jmooring/hugo-testing hugo-github-issue-10894
cd hugo-github-issue-10894
hugo server

Works great!

@jmooring
Copy link
Member

I haven't tried to reproduce the error you encountered, but I noticed that your config is probably not what you intended:

inlineDelimiters = [
  {open = "$", close = "$"},
  {open = "\\(", close = "$\\)"},  # should be {open = "\\(", close = "\\)"}
]

@jmooring
Copy link
Member

On the Hugo side of this (not the extension), the default config should include the four pairs of delimiters described here, with the ability to override to replace, remove, or add pairs of delimiters.

For example, if my markdown includes LaTeX mathematical expressions/equations, this should be all I need to do1:

markup:
  goldmark:
    extensions:
      passthrough:
        enable: true

If I want to avoid fighting with dollar signs in markdown (e.g., "the price is $2.00"), I should be able to do this:

markup:
  goldmark:
    extensions:
      passthrough:
        enable: true
        delimiters:
          inline:
            - ['\(','\)'] 
          block:
            - ['\[','\]']

Footnotes

  1. Assumes you are loading KaTeX or MathJax.

@jmooring
Copy link
Member

jmooring commented Jan 21, 2024

@j2kun Here is one of several posts that is triggering the error:

content/posts/2017-08-14-notes-on-math-and-gerrymandering.md

Specifically, this is the bit that is causing the problem:

\[Soapbox\] ... \[/Soapbox\]

Which can be reduced to:

\[x\] \[x\]

The error is not specific to your site or theme. It can be reproduced in a test case with:

---
title: foo
---

\[x\] \[x\]

or

---
title: foo
---

$$x$$ $$x$$

So, any time you have two or more passthrough blocks in the same markdown paragraph.

This test panics in passthrough/passthrough_test.go:

func TestExample27(t *testing.T) {
	input := `$$x$$ $$x$$`
	fmt.Println(Parse(t, input))
}

j2kun added a commit to j2kun/hugo that referenced this pull request Jan 22, 2024
@j2kun
Copy link
Contributor Author

j2kun commented Jan 22, 2024

I made the changes requested to the hugo extension:

  • Changed config structure
  • Added default delimiters for LaTeX math mode (and tested overrides)
  • Used simpler test invocation

Fixing the double-block bug in the parser next

@j2kun j2kun changed the title markup/goldmark: support latex fences $ and $$ markup/goldmark: support passthrough extension Jan 22, 2024
@j2kun
Copy link
Contributor Author

j2kun commented Jan 22, 2024

I think it should be good now, tested against gohugoio/hugo-goldmark-extensions@2ce467c with my whole blog and at least it doesn't crash. I will have to do some more investigating on my blog posts, because the Wordpress->hugo export chain I used did some funky stuff with my typeset math (e.g., all backslashes are escaped, all brackets are escaped, which led to that funky \[ Soapbox \] line, while my actual block math mode was converted to \\\[ ... \\\])

@jmooring
Copy link
Member

jmooring commented Jan 22, 2024

I wanted to disable the $ delimiters, both inline and block, so did this in site config:

[markup.goldmark.extensions.passthrough]
  enable = true

[markup.goldmark.extensions.passthrough.delimiters]
  block = [['\[', '\]']]
  inline = [['\(', '\)']]

But that has no effect when testing. When I type hugo config | grep passthrough -A3 I see:

[markup.goldmark.extensions.passthrough]
  enable = true

  [markup.goldmark.extensions.passthrough.delimiters]
    block = [['\[', '\]'], ['\[', '\]']]
    inline = [['\(', '\)'], ['\(', '\)']]

@jmooring
Copy link
Member

jmooring commented Jan 22, 2024

My previous test was invalid... this works great to disable the $ inline delimiter:

[markup.goldmark.extensions.passthrough.delimiters]
inline = [['\(', '\)']]

However, the stored config is incorrect:

hugo config | grep passthrough -A3

result

[markup.goldmark.extensions.passthrough]
  enable = true

  [markup.goldmark.extensions.passthrough.delimiters]
    block = [['$$', '$$'], ['\[', '\]']]
    inline = [['\(', '\)'], ['\(', '\)']]

@jmooring
Copy link
Member

jmooring commented Jan 22, 2024

@j2kun I think we should remove the single $ inline delimiter from the default configuration:

[markup.goldmark.extensions.passthrough.delimiters]
block = [['$$', '$$'], ['\[', '\]']]
inline = [['\(', '\)']]

Neither KaTeX.js nor MathJax.js enables the single $ inline delimiter by default. Our config would be consistent with theirs, and out-of-the-box users would not have to fight with single $ used in currency, etc.

Thoughts?

@jmooring
Copy link
Member

Related to a previous comment...

I expected that I would be able to disable inline parsing with this:

[markup.goldmark.extensions.passthrough.delimiters]
inline = []

But the config is unchanged.

$ hugo config | grep -A3 passthrough

[markup.goldmark.extensions.passthrough.delimiters]
block = [['$$', '$$'], ['\[', '\]']]
inline = [['$', '$'], ['\(', '\)']]

@j2kun
Copy link
Contributor Author

j2kun commented Jan 23, 2024

I think we should remove the single $ inline delimiter from the default configuration:

If the goal is to have an extension that is agnostic of LaTeX, then my thought is to have no default configured delimiters. Make the user explicitly set them always. It would cause fewer surprises for people who want to use it for something besides TeX math mode, and would result in less code on the hugo side.

If you disagree, I'll go ahead and fix both the issues above (defaults not cleared and remove $ as a default inline fence).

@jmooring
Copy link
Member

then my thought is to have no default configured delimiters

You're right; thank you. We have to document this functionality anyway, including how to add and remove delimiter pairs. Please proceed.

markup/goldmark/integration_test.go Outdated Show resolved Hide resolved
markup/goldmark/integration_test.go Outdated Show resolved Hide resolved
@jmooring
Copy link
Member

jmooring commented Jan 25, 2024

Also, please squash the commits with a new commit message:

markup/goldmark: Support passthrough extension

Fixes #10894

We capitalize the first word after the "foo/bar: " bit per:
https://github.com/gohugoio/hugo/blob/master/CONTRIBUTING.md#git-commit-message-guidelines

EDIT: bep intends to roll a release shortly, and it will include this PR when it goes green.

@j2kun
Copy link
Contributor Author

j2kun commented Jan 26, 2024

Address all comments.

@jmooring jmooring changed the title markup/goldmark: support passthrough extension markup/goldmark: Support passthrough extension Jan 26, 2024
@bep
Copy link
Member

bep commented Jan 26, 2024

This looks solid, great work!

@bep bep merged commit d0d2c67 into gohugoio:master Jan 26, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

markup/goldmark: Enable pass-through of raw content blocks
4 participants