Skip to content
This repository was archived by the owner on Aug 3, 2024. It is now read-only.

Add support for labeled module references #1078

Closed
wants to merge 1 commit into from

Conversation

garetxe
Copy link
Contributor

@garetxe garetxe commented Jul 31, 2019

This pull request adds support for a markdown-style way of annotating module references. For instance

-- | [label]("Module.Name#anchor")

will create a link that points to the same place as the module
reference "Module.Name#anchor" but the text displayed on the link will
be label.

One motivation for wanting this is detailed in #802 (comment): in the haskell-gi bindings for instance, I will be using this syntax to refer to the documentation for certain sections in the documentation, for example for signals and properties of objects. Something like the following

-- | ... when the button is clicked, the [clicked]("GI.Gtk.Objects.Button#signal:clicked") signal is emitted.

that would render to "... when the button is clicked, the clicked signal is emitted."

I can see this being useful more generally, of course, for instance if one would like to refer to the set of substring methods in Data.Text, one could use [substring methods]("Data.Text#g:17").

The pull request is currently missing unit tests and a change to the user guide, I would be happy to do those if you think that this feature is worth having. I have been using already in the gi-gtk bindings, and seems to work well in practice.

@harpocrates
Copy link
Collaborator

I am in support of this! It comes up often enough that having syntax for this is definitely desirable. I have only one concern, which is exemplified in the Data.Text snippet you mention:

I can see this being useful more generally, of course, for instance if one would like to refer to the set of substring methods in Data.Text, one could use [substring methods]("Data.Text#g:17").

If Data.Text adds a new section, g:17 may no longer point to substring methods! It may be that depending on which build plan gets picked during Haddock-ing, you end up with links pointing to completely different places. Even worse: what if a user links to an anchor that Haddock then ceases to generate (perhaps Haddock maintainers needed the anchor for some internal purpose)? To put the problem another way: is there perhaps some more precise syntax we could have for allowing only links to a subset of anchored things (which are at at least somewhat stable)?

Off the top of my head the following would be candidates:

Thoughts?

@garetxe
Copy link
Contributor Author

garetxe commented Jul 31, 2019

I am in support of this!

Great! :)

Thoughts?

I agree that the current way of linking to sections is not ideal. What I am doing right now is something like

-- ** activate #signal:activate#

which creates an anchor in the right place, and then I link to that using "Module#signal:activate". This sidesteps the issue you point out of the section names being unstable, which is in part why I did this. But another reason is that in the context of the bindings sometimes I do need the extra namespacing, since there are modules for which I have two subsections with the same name. For example, there is something called a GApplication, in the context of the gi-gtk bindings, which has both an activate method and an activate signal. Using signal: and method: namespaces I can disambiguate the links here.

So I would ask that we don't make the syntax too restrictive, I believe that there are valid cases in which one would like to link to arbitrary anchors.

That being said, I also tried to improve the way groups are anchored (although I will probably not be using this in haskell-gi, for the reason above). The following almost works (in `haddock-api/src/Haddock/Backends/Xhtml.hs):

nameSectionHeadings :: [ExportItem DocNameI] -> [ExportItem DocNameI]
nameSectionHeadings = go Set.empty
  where go :: Set String -> [ExportItem DocNameI] -> [ExportItem DocNameI]
        go _ [] = []
        go seen (ExportGroup lev _ doc : es)
          = let candidate = normalizeAnchor $ _ doc --- ?????
                -- _ :: DocH (Wrap (ModuleName, OccName)) (Wrap DocName) -> String
                groupName = buildNotInMap candidate seen
                newSeen = groupName `Set.insert` seen
            in ExportGroup lev groupName doc : go newSeen es
        go seen (other:es)
          = other : go seen es

-- | Find a version of the given string not in the given set, possibly
-- with some integer appended.
buildNotInMap :: String -> Set String -> String
buildNotInMap s set
  | s /= "" && s `Set.notMember` set = s
  | otherwise = buildUnique (0 :: Integer)
  where buildUnique n = let candidate = s ++ show n
                        in if candidate `Set.notMember` set
                           then candidate
                           else buildUnique (n+1)

-- | Given a string, construct a simplified version that works as an anchor.
normalizeAnchor :: String -> String
normalizeAnchor [] = []
normalizeAnchor (c:rest)
  | isAlphaNum c = c : normalizeAnchor rest
  | c `elem` "_ " = '_' : normalizeAnchor rest
  | otherwise = normalizeAnchor rest

but I am having some trouble figuring out how to fill in the hole. Any suggestions? I would be happy to send a separate patch with this, if I can figure out how to do it.

garetxe added a commit to haskell-gi/haskell-gi that referenced this pull request Aug 3, 2019
In particular, generated proper links to signals. Note that this needs
haddock with the pull request

haskell/haddock#1078

applied.
Support a markdown-style way of annotating module references. For instance

-- | [label]("Module.Name#anchor")

will create a link that points to the same place as the module
reference "Module.Name#anchor" but the text displayed on the link will
be "label".
@garetxe
Copy link
Contributor Author

garetxe commented Aug 6, 2019

I have now added tests and documentation to the pull request, so as far as I can tell this is now ready for review.

There is one point that I was not sure about: should we accept spaces between the parenthesis and the module name? Namely, is [label]( "Module" ) (note the spaces around the module name) OK? Currently we do accept this, and just trim the spaces away, so the above is equivalent to [label]("Module"), but we could also easily reject it. I thought accepting the spaces would be less surprising, but either way is fine, just let me know.

I also thought more about the point @harpocrates raised above, about restricting the possible form of the allowed anchors. Perhaps another argument in support of not restricting the anchors would be that currently arbitrary anchors can be created using the #anchor# syntax, and we probably want to have a way of linking to them.

Finally, about the point of generating better anchors for sections, I would be happy to take a look at it when I get some more time, but it is independent of the current patch, so I think that the current pull request could be considered independently.

@garetxe
Copy link
Contributor Author

garetxe commented Feb 18, 2020

Ping? Sorry for bothering, but I was wondering if this patch could be reviewed (and hopefully merged, if there are no issues)?

It has proven very useful and reliable to me, and I believe will be useful for others too, but I would prefer not having to depend on my own personal fork.

@Kleidukos
Copy link
Member

Superseded by #1181

@Kleidukos Kleidukos closed this Jan 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants