Python-Markdown / markdown

A Python implementation of John Gruber’s Markdown with Extension support.
https://python-markdown.github.io/
BSD 3-Clause "New" or "Revised" License
3.71k stars 856 forks source link

Abbr Extension: Definition Sorting and Glossary storage #1467

Closed nbanyan closed 2 months ago

nbanyan commented 2 months ago

Several changes/features here. All input welcome on each.

The glossary ended up being the more complex of the three and involved replicating some of the code/process from core.py.

waylan commented 2 months ago

So far I've only taken a cursory look at your changes. Here are my initial thoughts.

  • Sorting: A simple sort by length to process longer abbreviations first solves all instances of smaller abbreviations preventing longer ones from being used.

This is good. I should have caught the issue this fixes with the recent refactor. Thank you.

  • glossary: An option to include a Markdown file with abbreviations to apply to every page.

I'm not sure I am comfortable with this. In our discussion in #1465 I suggested allowing a user to pass in a Python object (such as a dict), not importing a file of Markdown content. I'm inclined to reject this on the basis that extensions already exist which allow Markdown content to be imported.

  • use_last_abbr: An option to only keep the first instance of an abbreviation (useful when using other methods to append glossaries to pages).

As previously explained, I don't see any value in this. You still need to ensure your definitions are in a certain order. That order is now reversed which is counterintuitive (IMO) and works the opposite of every other reference-type syntax (reference links, footnotes, etc) in Markdown. I realize this is an option which if off by default, but with there being no value to changing the setting, then why even offer it? That said, if you can provide a concrete example which demonstrates its value and changes my mind, then I would suggest renaming it use_first_abbr with the default being False. That name more clearly aligns with the description of the option.

nbanyan commented 2 months ago

Discarded use_last_abbr and changed glossary to accept and manage dictionary objects directly.

waylan commented 2 months ago

@nbanyan thanks for the work on this. Much appreciated.