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

Support multiple occurrences #37

Closed
glennreyes opened this issue Jul 30, 2019 · 6 comments · Fixed by #46
Closed

Support multiple occurrences #37

glennreyes opened this issue Jul 30, 2019 · 6 comments · Fixed by #46
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@glennreyes
Copy link

It would be nice if this would support multiple occurrences of same headings, similar to what https://github.com/Flet/github-slugger can do. So multiple occurrences are appended with an incremental number or perhaps hash.

Thoughts @sindresorhus?

@CanRau
Copy link
Contributor

CanRau commented Feb 1, 2020

That would be awesome 🥇

Supporting this means subsections can go by the same name. (for instance, in an md file I have, I have a section for specifications of files, and another section for examples of the files in action, and each section needs a subsection of the same name, as they're the same file)

From markedjs/marked#879 (comment)

@CanRau
Copy link
Contributor

CanRau commented Feb 1, 2020

Wanted to talk a little about the strategy if it's going to be implemented.
github-slugger is doing it like so

foo -> foo
foo -> foo-1
foo -> foo-2
..

I think it should start at 2 not at 1 cause it's effectively the second occurrence.

IMHO that reads better and fits better in the humanize-url philosophy. I think it makes it more predictable, like
"That anchor url/#example-2 refers to the second occurrence of ## Example"

Would love to know what you guys think. I can see it potentially conflict in cases where people want to replace github-slugger with slugify, maybe the counter could be configurable, defaulting to my suggestion 🤔

For now I'm using a little wrapper remark-plugin handling the counter anyways so it's actually not that urgent but would be a nice to have 💯

Update:
One thing I'm not sure about is how to handle

slugify.reset()
t.is(slugify('foo', { counter: true }), 'foo');
t.is(slugify('foo', { counter: true }), 'foo-2');
t.is(slugify('foo', { counter: true }), 'foo-3');
t.is(slugify('foo 2', { counter: true }), 'What should this be?');

I cloned the repo and implemented a simple "counter" behind an option as you might've guessed, mainly to not break the other tests..
Anyway, the last one feels a little tricky to me as it's not the same heading so naturally it should be foo-2 which of course doesn't work. 🤷‍♂

@CanRau
Copy link
Contributor

CanRau commented Feb 3, 2020

Okay I made a wrapper for internal consistency and to specify options once across projects and implemented a counter satisfying my current desires (I guess 🤔).

@gaiama/slugger

Those are all currently passing tests related to duplicates

slugify.reset();
t.is(slugify('foo'), 'foo');
t.is(slugify('foo'), 'foo-2');
t.is(slugify('foo 1'), 'foo-1');
t.is(slugify('foo-1'), 'foo-1-2');
t.is(slugify('foo-1'), 'foo-1-3');
t.is(slugify('foo'), 'foo-3');
t.is(slugify('foo'), 'foo-4');
t.is(slugify('foo-1'), 'foo-1-4');
t.is(slugify('foo-2'), 'foo-2-1'); // or foo-2-2 ??
t.is(slugify('foo-2'), 'foo-2-2');
t.is(slugify('foo-2-1'), 'foo-2-1-1');
t.is(slugify('foo-2-1'), 'foo-2-1-2');
t.is(slugify('foo-11'), 'foo-11-1');
t.is(slugify('foo-111'), 'foo-111-1');
t.is(slugify('foo-111-1'), 'foo-111-1-1');
t.is(slugify('fooCamelCase', { lowercase: false }), 'fooCamelCase');
t.is(slugify('fooCamelCase'), 'foocamelcase-2');

I'd like to know what you guys think about the numbering logic❓

By the way, I'm using this regex /(-\d+?)+?$/ and am not 100% sure it's ReDoS safe according to https://redos-checker.surge.sh it's fine with the ? and the tests on regex101.com are super fast, which is not always the case, yet that +?)+ is pretty suspicious but I don't know how else to handle this right now 🤔

@sindresorhus sindresorhus added enhancement New feature or request help wanted Extra attention is needed labels Feb 17, 2020
@sindresorhus
Copy link
Owner

I think it should start at 2 not at 1 cause it's effectively the second occurrence.

Agreed.

I can see it potentially conflict in cases where people want to replace github-slugger with slugify, maybe the counter could be configurable, defaulting to my suggestion 🤔

Nah. We can deal with that if anyone complains.

t.is(slugify('foo-2'), 'foo-2-1'); // or foo-2-2 ??

2-2, for consistency.


It cannot be the main slugify() method though as modifying it affects the whole process, not just your code. I'm thinking const countableSlugify = slugify.counter() to create a counter.


PR welcome.

@CanRau
Copy link
Contributor

CanRau commented Mar 4, 2020

Later than planned 😅

Got a little confused while working on a PR.

From my former example

slugify.reset();
t.is(slugify('foo'), 'foo');
t.is(slugify('foo'), 'foo-2');
t.is(slugify('foo 1'), 'foo-1');
t.is(slugify('foo-1'), 'foo-1-2');
t.is(slugify('foo-1'), 'foo-1-3');
t.is(slugify('foo'), 'foo-3');
t.is(slugify('foo'), 'foo-4');
t.is(slugify('foo-1'), 'foo-1-4');
t.is(slugify('foo-2'), 'foo-2-1'); // or foo-2-2 ??
t.is(slugify('foo-2'), 'foo-2-2');
t.is(slugify('foo-2-1'), 'foo-2-1-1');
t.is(slugify('foo-2-1'), 'foo-2-1-2');
t.is(slugify('foo-11'), 'foo-11-1');
t.is(slugify('foo-111'), 'foo-111-1');
t.is(slugify('foo-111-1'), 'foo-111-1-1');
t.is(slugify('fooCamelCase', { lowercase: false }), 'fooCamelCase');
t.is(slugify('fooCamelCase'), 'foocamelcase-2');

Does foo 1 -> foo-1 on line 4 make any sense here?
I mean it's not clashing but is it logical? 🤔
Should probably be foo-3 right?

I now think it's confusing in this "contrived" testing yet I think I actually prefer it as is 👍

@sindresorhus
Copy link
Owner

Does foo 1 -> foo-1 on line 4 make any sense here?

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants