-
Notifications
You must be signed in to change notification settings - Fork 129
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
correctly handle empty strings in charmaps #174
Conversation
This introduces a new way to remove characters, but there is already the slugify('cat', {remove: /a/ig}) // 'ct' Not necessarily saying we shouldn't do this, but we might want to pause before introducing multiple ways to do the same thing. If this is a more ergonomic way, consider deprecating |
@Trott they're not the same semantically. The "mapping into empty string" is related to the transliteration rules. It is a way to specify in the locale and/or charmap that we don't want a specific letter to be translated into anything. This is already in the charmap for The |
Thanks for the explanation. That makes sense and I understand now. |
delete require.cache[require.resolve('../')] | ||
slugify = require('../') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant to this PR, but slug
has a .reset()
method to make this more ergonomic. Maybe slugify
could do something similar.
This is only useful for the twenty six letters of the Latin alphabet, right? For everything else, it already works as expected? Or am I wrong about that? (EDIT: Maybe it's useful for certain common symbols too?) |
@Trott my own usecase was actually similar to the one described in #172: the Cyrillic characters The test uses latin characters for the sake of simplicity. |
I'm sorry if I'm being foolish here and missing something obvious, but doesn't that already work without the change in this pull request? const slugify = require('slugify');
const str = 'ъaъ';
console.log(slugify(str)); // 'uau'
slugify.extend({ 'ъ': ''})
console.log(slugify(str)) // 'a' Maybe all that needs to happen is to add a Russian locale to |
@Trott sorry for being so unclear :) The tricky part is: in your case, > var slugify = require('slugify')
undefined
> slugify('ъяъ', { remove: /[]/ })
'uyau'
> slugify.extend({ 'ъ': '' })
undefined
> slugify('ъяъ', { remove: /[]/ })
'ъyaъ' The order of steps for the current behavior is:
If someone that is not aware of language-specific rules specifies a custom |
@Trott we could instead just document that the |
I've updated the test to be a more relevant example of bugged behavior. |
I think I see now. FWIW, the const slug = require("slug");
const slugify = require("slugify");
const str = "ъяъ";
console.log(slug(str)); // 'uyau'
console.log(slugify(str)); // 'uyau'
slug.extend({ ъ: "" });
slugify.extend({ ъ: "" });
console.log(slug(str)); // 'ya'
console.log(slugify(str)); // 'ya'
console.log(slug("ъяъ", { remove: /[]/g })); // 'ya'
console.log(slugify("ъяъ", { remove: /[]/g })); // 'ъyaъ' |
Resolves #172
Resolves #95