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

Possibility to strip the keys from string #101

Closed

Conversation

iwollmann
Copy link

I have this need recently and realized that the strip function doesn't attended my needs, so I needed to make a regex to treat that on my project and I would love it so see that incorporated on this project.

Please, take a look and see if it's makes sense to have this here.

Basically it's remove a string that contains the emojis keys, e.g. I :love: cleaning -> I cleanning

@JoshuaKGoldberg
Copy link
Collaborator

JoshuaKGoldberg commented May 20, 2023

👋 @iwollmannl, new junior maintainer here picking up old threads - do you still want this? 😄

As of #113, functions such as strip that take in options do so with an options object. I can see this being maybe reasonable as an option like includeNames?: boolean (or a better name, I haven't thought deeply on that one). But then we'd probably want to add it to replace also, as strip just calls to replace...

I also question whether this is something node-emoji really needs to do? cc @omnidan

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label May 20, 2023
/**
* regex to remove emoji key strings, e.g. "":coffee: coffee" -> coffee
*/
var removeNameRegex = /:.*?\:/;
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg May 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Style] The ? is unnecessary. .*? says:

  • .: any character
  • *: any number of times (including 0)
  • ?: optionally

I'm betting the regular expression shouldn't match ::. So it can be simplified to:

Suggested change
var removeNameRegex = /:.*?\:/;
var removeNameRegex = /:.+:/;

it('Should be able to strip all emojis names', function() {
var result = emoji.strip('I :heart: cleaning and :coffee:', true);
result.should.equal('I cleaning and ');
});
Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg May 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Testing] Thinking about the test cases, there are some that should probably be added:

  • A string with :: somewhere in it
  • Colons that shouldn't be matched, like I: the great lover of coffee and I: coffee. You: tea.
  • A string that has emojis and :name:s

Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we'll end up wanting to take in this change (it's generally good to discuss feature additions in an issue first). But just in case, leaving a PR review. Hopefully this is helpful - or at least interesting! 😄

@omnidan
Copy link
Owner

omnidan commented May 21, 2023

@iwollmann if you're still interested in this, could you provide an example where such a functionality would be useful? trying to figure out if it should be a generally supported function or not. like you said, it's easy to do it with a regex replace.

@JoshuaKGoldberg
Copy link
Collaborator

Closing out since it's been a few months without response. Cheers!

@iwollmann
Copy link
Author

Hey @JoshuaKGoldberg I'm really sorry, this was hitting my spam box and I wasn't following it here anymore. To be honest I do not remember the actual case for this, but if you want I can update the PR with the requested changes.

@JoshuaKGoldberg
Copy link
Collaborator

JoshuaKGoldberg commented Sep 7, 2023

Ha, GitHub hitting spam - been there! No worries - I think the PR is more blocked on an explanation of why this would be useful to have in the library. #101 (comment)

@iwollmann
Copy link
Author

Nop, I remember that was something related with a markdown version and I had this need to strip the emoji format from the md text but can't remember in more details =/
It was an integration with slack...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting for author Needs an action taken by the original poster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants