sindresorhus / slugify

Slugify a string
MIT License
2.56k stars 81 forks source link

Support multiple occurrences #37

Closed glennreyes closed 4 years ago

glennreyes commented 5 years ago

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 commented 4 years ago

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 https://github.com/markedjs/marked/issues/879#issue-218109409

CanRau commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Yes