anchor-markdown-headings.js: fix IDs to be unique.

[XhmikosR]

Works fine, but I can't figure out the tests.

Any help is welcome.

Fixes #2440

[XhmikosR]

I think the proper solution would be to hook marked into the tests.

I might have a take at it tomorrow, but any help is welcome.

[XhmikosR]

Alright, I think we got it. Basically I only use marked in the duplicate IDs test and I check for slugger in our custom function.

The behavior should be the same, I just swapped name with id on the anchor so the generated anchors should be the same, except for cases with duplicate IDs where they didn't work anyway.

If anyone has any suggestions please let me know.

[Trott]

I think the proper solution would be to hook marked into the tests.

I might have a take at it tomorrow, but any help is welcome.

@phillipj @nodejs/website Anyone able to give @XhmikosR a hand with this?

[phillipj]

Reading the marked docs I get the impression that the slugger argument will always be provided when our custom header renderer function (anchorMarkdownHeadings()) is called by marked. Or have I misunderstood?

If it is indeed always provided, could we invoke slugger.slug() directly, without having logic checking if slugger is indeed provided? Primarily to help new contributors and our future selfs when trying to understand what this code does later, not having to graps unnecessary logic is valuable.

[phillipj]

Seems like you've found a decent way of testing the duplicate IDs by using marked in the tests 👍

Other than the raised question about invoking slugger.slug(), this LGTM.

[XhmikosR]

Thanks for the comments! The only case that slugger was not set was in tests. Feel free to push to my branch or suggest an edit.

…

On Fri, Aug 23, 2019, 22:39 Phillip Johnsen ***@***.***> wrote: Seems like you've found a decent way of testing the duplicate IDs by using marked in the tests 👍 Other than the raised question about invoking slugger.slug(), this LGTM. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2441?email_source=notifications&email_token=AACVLNKCW7OBX7LWE2LOIYTQGA4IRA5CNFSM4IOIGYQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BETYI#issuecomment-524437985>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACVLNP5LD3CR5DPKUE64NLQGA4IRANCNFSM4IOIGYQQ> .

[XhmikosR]

@MaledongGit: this wasn't supposed to be squashed either. I keep the patches separate on purpose in my PRs, when it makes sense to do so. Please do not squash my patched blindly.

[silverwind]

I think the default UI merge action is set to squash. Maybe add a note to the initial comment on PRs that should not be squashed.

[XhmikosR]

Yeah, the thing with the merge button, is that it remembers your last choice.