markup/goldmark: Add goldmark-cjk-friendly extension

[Martin005]

Add https://github.com/tats-u/goldmark-cjk-friendly extension which enhances handling of CJK emphasis and strikethrough.

Fixes #14114

[Martin005]

@tats-u Thank you so much for the review! ❤️ Your changes make total sense, I updated the code so that Hugo exposes only friendlyEmphasis and what option from your extension is picked based on that.

You can see the changes in this compare: https://github.com/gohugoio/hugo/compare/a615f42fe14c4878b534752edb1bdd4659261680..308ec733686b96fbb27fb68bc15fdba9110c8e39

Thanks again and if there is anything else I can improve, just let me know 🙂

[tats-u]

LGTM, waiting for reviews by maintainers.

[jmooring]

With this config:

[markup.goldmark.extensions]
strikethrough = true

[markup.goldmark.extensions.cjkFriendly]
emphasis = false

Both strikethrough and CJKFriendlyStrikethrough are appended to the slice of extensions. Is there a reason for that?

[tats-u]

It is due to:

[Martin005]

@jmooring

With this config:

[markup.goldmark.extensions]
strikethrough = true

[markup.goldmark.extensions.cjkFriendly]
emphasis = false

Both strikethrough and CJKFriendlyStrikethrough are appended to the slice of extensions. Is there a reason for that?

Thank you for noticing that. After discussion with @tats-u, I modified it as seen in https://github.com/gohugoio/hugo/compare/3f70a24bb44c54a29c08687ebe026d7b51795058..91227241d58eaabab548d52e712c36257f3184f8.

Can we get this PR merged now? I would love to have this feature in the next v0.153.0 release.

[jmooring]

OK, this seems to work correctly, but the extensions config is difficult to reason about. We now have three extensions to handle strikethough:

1. Strikethrough
   • config exposed to user
   • automatically disabled if cjkFriendly.Emphasis is enabled
   • must manually disable if extras.Subscript is enabled
2. cjkFriendly.Strikethrough
   • config not exposed to user
   • automatically enabled when cjkFriendly.Emphasis and Strikethrough setting is true
3. extras.Delete
   • config exposed to user
   • must manually enable if you want strikethrough and subscript behavior concurrently

And I think which one is actually used depends on the parse/render priorities, settings that are not exposed to the user.

This needs to be thought through...

1. Hide the extras.Delete config, leaving only the top-level Strikethrough setting exposed
2. If extras.Subscript and Strikethrough are enabled, enable extras.Delete (don't enable the other two)
3. If cjkFriendly.Emphasis and Strikethrough are enabled, and extras.Subscript is not enabled, enable cjkFriendly.Strikethrough (don't enable the other two)
4. If neither extras.Subscript nor cjkFriendly.Emphasis is enabled, and Strikethrough is enabled, use the Strikethrough extension (don't enable the other two)

Someone should probably create a quick flow chart before coding this. It makes my head hurt.

EDIT...

• We need to make sure whatever we do is backwards compatible, or at least be aware of and note the breaking change
• Should include deprecation warning if extras.Delete is in site config

[Martin005]

@jmooring To be honest, I don't understand why we would need to hide the extras.Delete option from config.
If the user wants to enable that option only, they should be able to do so. If they want Strikethrough, let them have it.
And if they need to build multilingual site with CJK languages, let them enable Strikethrough and the CJK-friendly strikethrough.

Is there anything I can help you with to push this PR to being merged? Thanks!

[tats-u]

https://github.com/gohugoio/hugo-goldmark-extensions?tab=readme-ov-file#deleted-text

[tats-u]

https://gohugo.io/configuration/markup/#extensions

Strikethrough is enabled by default. It's a pain in the neck to disable it by hand.

[tats-u]

There should be no reason to prefer Delete to Strikethrough unless you combine it with Subscript.

[bep]

Don't change this PR until we agree on something, but my take on this is that:

• It's very hard to create a rule set that disables extension X when Y and Z is enabled, and there will be many unhappy users even if we find something reasonable.
• CJK language writers seem to really care about their language and getting the nuances about the typography etc. correct (which I admire, we here in Norway are sloppy). So, I suspect that they don't mind spending some time tweaking the config to get it just right.

With that said, how about if we, given the pseudo config setup below, created something like:

[extensions]
[extensions.delete]
disable = false
weight = 100
[extensions.strikethrough]
disable = true
weight = 200
[extensions.cjk]
disable = true
weight = 300
[extensions.cjkfriendly]
disable = true
weight = 400

I have not given much thought to the defaults in the above, but if we used any non-zero weight set as the priority of the extension, then the end user should relatively easy get exactly the behavior he/she wants. And if he/she wants to shoot here/himself in the foot, so be it.

[tats-u]

It's very hard to create a rule set that disables extension X when Y and Z is enabled, and there will be many unhappy users even if we find something reasonable.

This case is an exception that X-Z are mutually exclusive and parent-child relationships.

So, I suspect that they don't mind spending some time tweaking the config to get it just right.

They are also humans. The less amount of config needs to be set to achieve a desired behavior, the better it is.

[bep]

They are also humans. The less amount of config needs to be set to achieve a desired behavior, the better it is.

Fair enough, I was just trying to nudge this PR to a mergeable state.

[tats-u]

@Martin005

(⚠) This branch has conflicts that must be resolved

[kivikakk]

Per bep's earlier comment, it's probably not wise/not worth changing or updating this PR until an agreement on the direction is reached, otherwise any work might be wasted.

[tats-u]

I see.

[Martin005]

Hi @jmooring @bep. Is it possible to get this MR merged in the current state? If not, what would you need me to modify to get it merged? Thanks! 🙂

[tats-u]

It is a better idea to merge main instead of rebasing it once you get reviewed.

[bep]

@Martin005 sorry for the delay; there needs to be some consensus as to what the default config should be (read: A review from jmooring). It's much better to get that right before people start using it. Both me and him is a little bit busy on other fronts this week, so this may take a little time. But we will eventually get there.

[tats-u]

You should stop force-push. It will make it difficult to review your changes.

[tats-u]

Conflicts appear again