-
Notifications
You must be signed in to change notification settings - Fork 637
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
Support in-line glossary definitions and references with an index #12150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the result but I'm a bit uncomfortable reinventing an existing feature rather than just extending it. Wouldn't there be a way to get the same result but by piggy-backing on the current glossary infrastructure (in particular reusing :term:
instead of :gref:
) like prodn
and :n:
piggy-back on the existing token mechanism (so that :production:
and :token:
remain available and compatible)?
Ultimately this is more the area of expertise of @cpitclaudel than mine.
The implementation looks reasonable, but I would have reused the existing glossary objects; basically you want to define glossary entries inline, right? |
AFAIU, that + a specific index of glossary objects. |
It's easy to switch to using I'd like to generate the glossary index using data from the For consistency, I think it makes more sense to have the glossary index defined in the Also FWIW note that the general index (coming from |
6d1f6bb
to
be87143
Compare
Updated. |
Not sure why |
For the record, the build succeeded with Dune and failed with make. Parallel builds have been disabled for the documentation with both build systems. |
@cpitclaudel Can you be the assignee for this PR? |
This version works. I tried to avoid the global by adding I'll squash and remove the |
That's correct. |
Changed the global to a static field of CoqDomain. A bit cleaner. |
@jfehrle The output contains a bunch of warnings that were not there before:
|
I don't see these messages locally when I build using Seems like the most likely explanation is a change in docutils, perhaps in Sphinx and/or Sphinx parameters. A few other thoughts toward understanding the AFAICT it's not something we can fix by changing the Python code in our source tree. |
OK sorry for the false alarm. |
ping @cpitclaudel: I feel like you are much more qualified than I am to review this PR. |
OK, I'll read through it today or tomorrow, I hope. |
@cpitclaudel I'll need to make a 1-2 line fix to support cross-file hyperlinks for terms with spaces in them. Just need to map the space to something else, like a dash. |
Doesn't the standard Sphinx url escaping do this? |
Actually doesn't matter; the link names Sphinx generates for a :term: substitute dashes for spaces. If we want them to find our :gdef: definitions then gdef has to do this as well. It's a one line fix. Squashed, removed the examples in the rst files and updated. |
@jfehrle You need to apply this patch: diff --git a/doc/sphinx/README.rst b/doc/sphinx/README.rst
index 0802b5d0b4..e20469bb8b 100644
--- a/doc/sphinx/README.rst
+++ b/doc/sphinx/README.rst
@@ -358,6 +358,13 @@ In addition to the objects and directives above, the ``coqrst`` Sphinx plugin de
<http://www.sphinx-doc.org/en/stable/markup/para.html#directive-productionlist>`_
and reference its tokens using ``:token:`…```.
+``:gdef:`` Marks the definition of a glossary term inline in the text. Matching :term:`XXX`
+ constructs will link to it. The term will also appear in the Glossary Index.
+
+ Example::
+
+ A :gdef:`prime` number is divisible only by itself and 1.
+
Common mistakes
=============== |
@Zimmi48 Yeah, I removed too much last night, thanks. But it was easier to use Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I just have one doubt about capturing env and adding the glossary as an index of the Coq domain.
I expect Sphinx uses nodes.make_id internally, and that's what we should do to (it escapes more than spaces) |
with an index
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great this way. Should I merge?
I think so, and this will allow me to not depend on any other PR in #12172. |
Here's an alternate implementation of a glossary. This has inline definitions (
:gdef:
) showing in italic and references (:gref:
) and aGlossary Page
index. An alternative to #12114. Look at the beginning of the implicit arguments section of the Gallina extensions chapter. And the new index.What do you think?