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

✨ New: add .glossary for attrs_block #719

Merged
merged 2 commits into from
Feb 23, 2023
Merged

✨ New: add .glossary for attrs_block #719

merged 2 commits into from
Feb 23, 2023

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Feb 23, 2023

MyST follows http://johnmacfarlane.net/pandoc/README.html#definition-lists, to denote lists of term -> definition.

Term 1
: Definition

Term 2
: Definition

It should be easy for users to turn one of these lists into a glossary,
where each term is uniquely referenceable across the entire project.
(note I don't think this should be "on by default", because there are many use cases where this is not desirable and would lead to many reference clashes, plus that would be breaking in myst-parser and so not really viable)

Sphinx has a glossary directive, however, this hard-codes in the parsing of their definition lists to have a subtly different syntax to that above, with no :
(see https://github.com/sphinx-doc/sphinx/blob/30f347226fa4ccf49b3f7ef860fd962af5bdf4f0/sphinx/domains/std.py#L354)

:::{glossary}
Term 1
    Definition

Term 2
    Definition
:::

Therefore, I feel that it is not really ideal for MyST.

The easiest way to achieve this with the "correct" syntax, as implemented in this PR, is to "turn on" glossaries, when a particular attribute is assigned to the list, with the simplest being a .glossary class:

{.glossary}
Term 1
: Definition

Term 2
: Definition

@chrisjsewell
Copy link
Member Author

cc @rowanc1 @choldgraf for comment

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Base: 89.81% // Head: 89.93% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (f191463) compared to base (7fe6411).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
+ Coverage   89.81%   89.93%   +0.12%     
==========================================
  Files          23       23              
  Lines        2887     2892       +5     
==========================================
+ Hits         2593     2601       +8     
+ Misses        294      291       -3     
Flag Coverage Δ
pytests 89.93% <100.00%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
myst_parser/mdit_to_docutils/base.py 92.94% <100.00%> (+0.03%) ⬆️
myst_parser/mdit_to_docutils/transforms.py 90.78% <0.00%> (+3.94%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@choldgraf
Copy link
Member

From a UX perspective this makes sense to me. I have found myself expecting definition lists to act like glossaries in the past and this would reduce the friction there.

A couple thoughts:

  • we should document this in that section - i didn't see it in there w/ the new glossary syntax in a literal block
  • is it possible to have multiple words per definition? I have found this is useful if you want to use definitions for acronyms as well as full names or plural versions

@chrisjsewell
Copy link
Member Author

we should document this in that section - i didn't see it in there w/ the new glossary syntax in a literal block

it's in the new documentation: https://myst-parser--717.org.readthedocs.build/en/717/syntax/typography.html#definition-lists-and-glossaries

@choldgraf
Copy link
Member

choldgraf commented Feb 23, 2023

Ah, so this is documented but not implemented, and this PR only adds the implementation? I was looking at the diff here and just didn't see any docs added which was confusing. Go for it though if it's already documented.

@chrisjsewell
Copy link
Member Author

is it possible to have multiple words per definition? I have found this is useful if you want to use definitions for acronyms as well as full names or plural versions

it would be nice for sure, but I'm not sure how to do it, in a "non-hacky way", with the current syntax 🤔

@choldgraf
Copy link
Member

IMO that's a fine case for "if you want more flexibility then just use the glossary directive". I feel like the short-hand ways are time savers but it's understandable to ask people to use more verbose syntax if they want the extra functionality, parameterization, etc

@chrisjsewell
Copy link
Member Author

Ah, so this is documented but not implemented, and this PR only adds the implementation?

This commit is cherry-picked from #717

With all the recent commits, I've basically been implementing things as I was writing that documentation. Then gradually extracting out individual implementations into separate PRs and re-basing

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Feb 23, 2023

Note also there is an issue tracking acronyms in jgm/djot#51, but it hasn't come to much yet, they mention there https://www.overleaf.com/learn/latex/Glossaries, and I have long while back written a sphinx extension to this effect: https://ipypublish.readthedocs.io/en/latest/sphinx_ext_bibgloss.html
That feels like more the "expansive way" to do acronyms

@choldgraf
Copy link
Member

Yeah the UX around acronyms and plurals is kinda wonky, but IMO that shouldn't block this PR and it's a future thing to think about.

@chrisjsewell
Copy link
Member Author

Ok cool, thanks for the feedback! Will wait for @rowanc1 before merging

@rowanc1
Copy link
Member

rowanc1 commented Feb 23, 2023

Pointed here from the big release where I asked to not to have class-names on paragraphs become admonition directives. I don't have any of those concerns here -- I think that is because we aren't changing the semantic meaning of the content, it is going from a definition, to a tagged definition, and so the short hand {.glossary} seems pretty low-risk and makes a lot of sense. (similar to going from a div, to a styled div with a ".callout-note" or something, it is upgrading semantics, not changing them).

I might have a few more opinions when I get to an implementation on glossaries myself, e.g. on plurals or adding other info. Some of that could be inline spans/roles on the term, or encouraging users to use the directive. All of that can be done later though!

Also agree that this shouldn't be turned on by default (probably ever) to transform dl/dd/dt into glossary terms.

@chrisjsewell
Copy link
Member Author

Thanks @rowanc1 !

To note, another point of reference is: jgm/djot#128

@chrisjsewell chrisjsewell merged commit 7692f87 into master Feb 23, 2023
@chrisjsewell chrisjsewell deleted the glossary-attr branch February 23, 2023 17:46
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.

3 participants