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

Allow custom extensions #7

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Allow custom extensions #7

merged 2 commits into from
Nov 3, 2022

Conversation

fmatter
Copy link
Collaborator

@fmatter fmatter commented Nov 1, 2022

I figured the right place for passing custom markdown extensions would be the settings, too. This works for me; as indicated in #5, some of the extensions that are currently included by default may not be wanted/needed by everybody.

@fmatter fmatter requested a review from xrotwang November 1, 2022 23:11
res[key].update(custom.get(key, {}))
elif key == "extensions":
res[key].extend(custom.get(key, []))
Copy link
Member

Choose a reason for hiding this comment

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

This will only allow adding to the defaults. Is this intended - i.e. could it be that a user explicitly doesn't want any of the default extensions?

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make sense to limit the default extensions to those that are actually needed to render whatever default renderers spit out (and maybe a short list of cross-platform well-supported things like fenced_code and tables). E.g. md_in_html or footnotes (and possibly attr_list) might be a bit too much.

Maybe this list could serve as some guideline: https://github.com/Crissov/CommonMark/blob/extensions/extensions/Desirable%20and%20popular%20Commonmark%20extensions.md

See also https://talk.commonmark.org/t/extension-terminology-and-rules/1233

Copy link
Member

Choose a reason for hiding this comment

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

We could also take the stance that pandoc is a key tool to make CLDF markdown useful, thus limit inclusion of default extensions to ones available for pandoc as well (see https://pandoc.org/MANUAL.html#pandocs-markdown). I.e. we shouldn't encourage people to tailor CLDF markdown to a particular renderer (and if we do, that renderer should be pandoc rather than the clld-markdown-plugin). The main idea of CLDF is that datasets will outlive web apps - after all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will only allow adding to the defaults. Is this intended - i.e. could it be that a user explicitly doesn't want any of the default extensions?

I found it hard to come up with specific cases for that scenario, but I guess it could be -- e.g. if they use some non-standard custom extension that would interfere with a default.

Would a reasonable solution be to overwrite the defaults by using

config.registry.settings["clld_markdown_plugin"]["extensions"] = [OnlyMyCustomExtension()]

after config.include("clld_markdown_plugin")? It does work :)

I think if a desired output is LaTeX (for printing), then pandoc conformity makes sense.

"markdown.extensions.attr_list",
"markdown.extensions.footnotes",
]
extensions=[TocExtension(permalink=permalink)] + settings["extensions"]
Copy link
Member

Choose a reason for hiding this comment

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

I think extensions that need parameters should always be specified as custom extension in the settings (initialized python objects should work). Otherwise we'd make the call signature of markdown too intransparent.

Copy link
Member

Choose a reason for hiding this comment

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

So the permalink keyword of markdown could be removed as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that's only there as a leftover from this comment. Will remove it.

Copy link
Collaborator Author

@fmatter fmatter left a comment

Choose a reason for hiding this comment

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

How about this?

@xrotwang xrotwang merged commit 661218f into main Nov 3, 2022
@xrotwang xrotwang deleted the florian branch November 3, 2022 06:36
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.

2 participants