Don't overwrite existing IDs

[WhiteWinterWolf]

Pelican-TOC currently overwrites existing IDs.

Among other things, this produces unexpected IDs when there are nested titles.

In the following markdown code:

# Title A
## Title 1
# Title B

The last title currently gets assigned the ID #title-b_1 while it should be #title-b.

The fix makes Pelican-TOC to generate IDs only for tags which do not have any ID yet, existing IDs are kept.

[ingwinlu]

sorry this won't do as you will have to make sure ids is populated correctly if you don't genarate all id's via the unique function (thats why I made the effort on documenting it # make sure id is unique)

[WhiteWinterWolf]

I agree with you for the new IDs generated by this module. However, IMHO:

1. This module should not overwrite IDs already set.

   When an ID is already assigned to an element, either manually or by a previous module, overwriting it will most likely break something. This module should therefore not affect already existing IDs.

2. It should have a consistent behavior.

   Let's take this initial construct:

    # Title A
    # Title B
   

   Now let's add a subtitle below A:

    # Title A
    ## Title 1
    # Title B
   

   This changes the ID affected to Title B from title-b to title-b_1. I don't think this the expected behavior.

   In fact, the module seems to parse some tags several times:

   • On the first pass it checks if the ID title-b is free, which it is, so it correctly affects the ID title-b to the Title B element.
   • On the second pass on the very same element it notices that that the ID title-b is now already existing (as it is precisely used by the exact tag the module is currently checking), so this module takes the decision to replace it with a title-b_1 tag.

   Again I don't think this is the expected behavior.

3. The IDs generated by this module should be consistent with the IDs generated by the original Table of Contents Markdown extension.

   In the case described above, the original extension still generates permalinks pointing to the title-b ID while this module generated a title-b_1 ID instead.

All this results in this module breaking existing links.

In my proposed patch the module still checks that newly generated IDs are unique, but it simply do not touch any already existing ID, solving all the issues mentioned here.

[ingwinlu]

except links would still be broken...

…

On 28 September 2017 at 10:34, WhiteWinterWolf ***@***.***> wrote: I agree with you for the new IDs generated by this module. However, IMHO: 1. This module should not overwrite IDs already set. When an ID is already assigned to an element, either manually or by a previous module, overwriting it will most likely break something. This module should therefore not affect already existing IDs. 2. It should have a consistent behavior. Let's take this initial construct: # Title A # Title B Now let's add a subtitle below A: # Title A ## Title 1 # Title B This changes the ID affected to *Title B* from title-b to title-b_1. I don't think this the expected behavior. In fact, the module seems to parse some tags several times: - On the first pass it checks if the ID title-b is free, which it is, so it correctly affects the ID title-b to the *Title B* element. - On the second pass on the very same element it notices that that the ID title-b is now already existing (as it is precisely used by the exact tag the module is currently checking), so this module takes the decision to replace it with a title-b_1 tag. Again I don't think this is the expected behavior. 3. The IDs generated by this module should be consistent with the IDs generated by the original *Table of Contents* Markdown extension. In the case described above, the original extension still generates permalinks pointing to the title-b ID while this module generated a title-b_1 ID instead. All this results in this module breaking existing links. In my proposed patch the module still checks that newly generated IDs are unique, but it simply do not touch any already existing ID, solving all the issues mentioned here. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEOv-g6WVfOhIOLKd0Nr2tfhD4Kb_OEvks5sm1owgaJpZM4PkZ9i> .

[WhiteWinterWolf]

Hi,

I'm not sure I understand your latest answer.

Let's say I have a link to https://example.com/some/path/#title-b

• Current behavior: Headers ID can be mangled and Title B header may be overwritten with title-b_1, making the above link indeed broken.
• Fixed behavior: Headers ID are not mangled, Title B header has the expected title-b ID.

Can you provide me more concrete information, like a test case or something like that where the fix breaks something?

[ingwinlu]

because there could be two identical ids. which would not just be unexpected but plain breaking.

i think the best part would be to build up all_ids during the tree generation and only when folding the tree into a string there should be new ones generated for missing ids. the effort was not worth it for me in the past (i like unique headers, don't necessarily care how they look) so i just made sure every header passes through unique.