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

Add counter for multiple occurences #46

Merged
merged 20 commits into from
May 7, 2020

Conversation

CanRau
Copy link
Contributor

@CanRau CanRau commented Mar 4, 2020

Closes #37

That's actually what I already made for @gaiama/slugger

only issue so far is, it's not doing the by you preferred -2-2 😢

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

and I'm currently not sure how to properly do it without a rewrite.
I'm open to other implementations or preferences.

index.d.ts is probably not correct? Still very new to TS and external declarations.

Also I'm not sure if the used regex /(?:-\d+?)+?$/ is ReDoS safe or how much that matters here 🤷‍♂

@sindresorhus
Copy link
Owner

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.

The counter cannot be global. It needs to create a new instance.

@CanRau
Copy link
Contributor Author

CanRau commented Mar 11, 2020

Not sure I understand, the current implementation is used like so

const countableSlugify = slugify.counter();
t.is(countableSlugify('foo bar'), 'foo-bar');

@sindresorhus
Copy link
Owner

https://github.com/sindresorhus/slugify/pull/46/files#diff-168726dbe96b3ce427e7fedce31bb0bcR71 is global. If slugify is used in multiple places in the dependency tree, they will share this, which will cause really weird problems.

@CanRau
Copy link
Contributor Author

CanRau commented Mar 11, 2020

You're right 😅
That should fix it, I've added tests for it as well.
Is that how you "envisioned" it?

index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Yes, perfect. That's exactly what I had in mind.

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 12, 2020

Can you add docs (readme + TypeScript file)? And also mention some use-cases in the docs.

@sindresorhus sindresorhus changed the title feat: add counter for multiple occurences Add counter for multiple occurences Mar 12, 2020
@CanRau
Copy link
Contributor Author

CanRau commented Mar 13, 2020

I updated everything as good as I could.
Please let me know if that works for you.

@CanRau
Copy link
Contributor Author

CanRau commented Apr 3, 2020

Should the countableSlugify count empty strings like

countableSlugify(""); // => ""
countableSlugify(""); // => "-2"

or should it always return "" ? 🤔

index.d.ts Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
readme.md Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

or should it always return "" ? 🤔

This. Add tests for that.

@sindresorhus
Copy link
Owner

Make sure $ npm test passes locally.

@CanRau
Copy link
Contributor Author

CanRau commented Apr 26, 2020

Thanks for the reminder, I only ran npx ava --watch yesterday and forgot about xo & tsd 😬

index.d.ts Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Looks great now. Thanks for working on this 👍

@sindresorhus sindresorhus merged commit 9e1ec95 into sindresorhus:master May 7, 2020
@CanRau
Copy link
Contributor Author

CanRau commented May 7, 2020

Thanks a lot for your patience & guidance @sindresorhus very much appreciated 🙏
I learned a lot 🎉

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.

Support multiple occurrences
2 participants