Skip to content

Commit

Permalink
Improve emoji lookup fallback behavior
Browse files Browse the repository at this point in the history 
Instead of an unfriendly LocalJumpError, raise the Emoji::NotFound
exception with a helpful message if the block was not given.

If a fallback block was given, yield the value for which the lookup failed.
  • Loading branch information
@mislav
mislav committed Jun 27, 2014
1 parent 70db910 commit 5d24012
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 9 deletions.
14 changes: 12 additions & 2 deletions lib/emoji.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
module Emoji
extend self

NotFound = Class.new(IndexError)

This comment has been minimized.

Sign in to view

Copy link
@jeremy

jeremy Jul 17, 2014

Contributor

IndexError is used for out of bounds indexes on Arrays. Probably want to subclass KeyError for a map lookup, or just use StandardError.

This comment has been minimized.

Sign in to view

Copy link
@mislav

mislav Jul 17, 2014

Author Contributor

I didn't want to subclass KeyError because it doesn't exist on 1.8, but later I realized gemoji isn't 1.8 compatible anyway.

def data_file
File.expand_path('../../db/emoji.json', __FILE__)
end
Expand All @@ -17,11 +19,19 @@ def all
end

def find_by_alias(name)
names_index.fetch(name) { yield }
names_index.fetch(name) {
if block_given? then yield name
else raise NotFound, "Emoji not found by name: %s" % name.inspect
end
}
end

This comment has been minimized.

Sign in to view

Copy link
@jeremy

jeremy Jul 17, 2014

Contributor

Example of a more familiar API style, including a second arg for default value:

def find_by_alias(name)
  fetch_by_alias name, nil
end

ALIAS_MISSING = lambda { |name| raise NotFound, "Emoji not found by name: %s" % name.inspect }
def fetch_by_alias(name, *default_value, &default_block)
  default_block ||= ALIAS_MISSING
  names_index.fetch(name, *default_value, &default_block)
end

This comment has been minimized.

Sign in to view

Copy link
@mislav

mislav Jul 17, 2014

Author Contributor

Example of a more familiar API style

I can be on board with this, sure. Please send a PR and we could make it v2.1 or something

This comment has been minimized.

Sign in to view

Copy link
@jeremy

jeremy Jul 18, 2014

Contributor

Done: #53

def find_by_unicode(unicode)
unicodes_index.fetch(unicode) { yield }
unicodes_index.fetch(unicode) {
if block_given? then yield unicode
else raise NotFound, "Emoji not found from unicode: %s" % Emoji::Character.hex_inspect(unicode)
end
}
end

def names
Expand Down
16 changes: 9 additions & 7 deletions test/emoji_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,15 @@ class EmojiTest < TestCase
end

test "emoji alias not found" do
assert_raises LocalJumpError do
error = assert_raises Emoji::NotFound do
Emoji.find_by_alias('$$$')
end
assert_equal %(Emoji not found by name: "$$$"), error.message
end

test "emoji by alias fallback block" do
emoji = Emoji.find_by_alias('$$$') { :not_found }
assert_equal :not_found, emoji
emoji = Emoji.find_by_alias('hello') { |name| name.upcase }
assert_equal 'HELLO', emoji
end

test "fetching emoji by unicode" do
Expand All @@ -39,14 +40,15 @@ class EmojiTest < TestCase
end

test "emoji unicode not found" do
assert_raises LocalJumpError do
Emoji.find_by_unicode('$$$')
error = assert_raises Emoji::NotFound do
Emoji.find_by_unicode("\u{1234}\u{abcd}")
end
assert_equal %(Emoji not found from unicode: 1234-abcd), error.message
end

test "emoji by unicode fallback block" do
emoji = Emoji.find_by_unicode('$$$') { :not_found }
assert_equal :not_found, emoji
emoji = Emoji.find_by_unicode("\u{1234}") { |u| "not-#{u}-found" }
assert_equal "not-\u{1234}-found", emoji
end

test "emojis have tags" do
Expand Down

3 comments on commit 5d24012

@jeremy
Copy link
Contributor

@jeremy jeremy commented on 5d24012 Jul 17, 2014

Choose a reason for hiding this comment

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

I found this API style pretty surprising, since I expect a missing lookup to return nil for nice operation with truthy conditionals, like

if emoji = Emoji.find_by_alias($1)
  ...
else
  ...
end

Raising an exception when the alias is missing does not feel actually exceptional—it's an expected common case. By raising, developers will often end up rescuing the exception for control flow rather than looking deeper to see that find_by_* has Hash#fetch-like semantics.

@josh
Copy link
Contributor

@josh josh commented on 5d24012 Jul 17, 2014

Choose a reason for hiding this comment

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

@jeremy yeah, I agree, this is kinda a smell.

@mislav
Copy link
Contributor Author

@mislav mislav commented on 5d24012 Jul 17, 2014

Choose a reason for hiding this comment

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

I wanted a nil-free API. Agreed that it makes the truthy conditional a little more awkward by forcing you to append a { nil } block. I don't expect developers to start using these exceptions as control flow, because if they do, that's their own fault. I think in some cases an exception is in order if the user think they're sure that an emoji by that name exists, but for some reason it doesn't (or gets removed in the future).

Suggestions for another API are welcome, but we'd have to do another major or minor release with this. v2.0 is already out.

Please sign in to comment.