jsumners / alfred-emoji

Alfred workflow for searching and copying emoji
741 stars 32 forks source link

Added support for copying codepoints (issue #57) #80

Closed sz-hankus closed 2 years ago

sz-hankus commented 2 years ago

The only changes besides the readmes and such is adding the codepoint for each emoji to 'unicode-emoji-json' in genpack.js. In order to avoid representing e.g. 5️⃣ as "U+35", I also wrote a function that adds leading zeroes to the codepoint so that the output looks as follows: "U+0035".

Here's a screenshot of the feature in action:

image

Let me know what you think :)

sz-hankus commented 2 years ago

I can see that some tests have failed, saying ERROR: Coverage for lines (97.95%) does not meet global threshold (100%). But when I cloned the original repo without my changes, the error was still there when I ran npm run test. Should I change something?

jsumners commented 2 years ago

You should be able to run tap --coverage-report=html to get an overview of the missing coverage. I'll try to find some time to examine the coverage on the master branch soon. It should be 100%.

sz-hankus commented 2 years ago

It seems that the problematic part is line 64 in search.js

https://github.com/jsumners/alfred-emoji/blob/7658e90f79dc59dbdbf21dc6e9cf789b4991fba5/src/search.js#L59-L65

It's kind of hard to test, because in order for a test to reach this line, emojiDetails need to be undefined. But from what I've tested, emojiInfo[char] is always defined for all emojis (at least on my system).

The obvious solution would be to add alfredItem to the module exports and just test it by directly calling it with undefined as arguments, but I'm pretty sure that it's not the prettiest solution.

jsumners commented 2 years ago

The obvious solution would be to add alfredItem to the module exports and just test it by directly calling it with undefined as arguments, but I'm pretty sure that it's not the prettiest solution.

That's totally fine. Would you like to submit a PR that adds the following, along with a test, to the end of search.js?:

module.exports.internals = {
  alfredItem
}
sz-hankus commented 2 years ago

Sure, I'll get on it.

sz-hankus commented 2 years ago

I think it should be ready to merge now