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

Fix search for occur to account for _n headings #238

Merged
merged 1 commit into from
Sep 29, 2019

Conversation

TotalVerb
Copy link
Contributor

Currently, only the first duplicated heading will have its ID renumbered, because the search for occur only looks for headings with the same ID. All of the duplicate headings after the first will be called heading_2:

### Heading (heading)
### Heading (heading_2)
### Heading (heading_2)

This changes the logic to look for all headings with the same refstring. There is probably still a problem if the user creates a document like

### Heading 2 (heading_2)
### Heading (heading)
### Heading (heading_2)

since the search won't find the explicit heading_2 refstring. I'm not sure how to address this cleanly, however, so I opted for a more minimal change.

@tlienart
Copy link
Owner

tlienart commented Sep 29, 2019

I guess one way to go is to do something a bit like in Documenter where the first occurence gets a _1 and then _2 etc etc so in the pathological situation you suggest:

### Heading 2 (heading_2_1)
### Heading (heading_1)
### Heading (heading_2)

Do you want to give it a shot? otherwise I'm happy to merge your contribution and tackle this later on. Thanks a lot!

@tlienart tlienart mentioned this pull request Sep 29, 2019
67 tasks
@TotalVerb
Copy link
Contributor Author

Yeah, Documenter.jl always numbers its links. But making that change would break existing section links generated by JuDoc, and in my opinion makes the links less understandable in the common case.

Another option is to reserve some sequence like double underscore __ for numbering, and make refstring collapse double underscore to single underscore, which would probably be a little less disruptive?

@tlienart
Copy link
Owner

Sounds good. I'll merge this and open an issue with your suggestion for consideration, thanks!

@tlienart tlienart merged commit 7a4e9ec into tlienart:master Sep 29, 2019
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