duckduckgo / zeroclickinfo-goodies

DuckDuckGo Instant Answers based on Perl & JavaScript
https://duckduckhack.com/
Other
979 stars 1.76k forks source link

Unicode: Throws an error on the query "unicode girls" #1430

Closed moollaza closed 8 years ago

moollaza commented 9 years ago

While testing in DuckPAN I noticed that this IA returns an undef result for the query "unicode girls". This should be fixed asap!

I suspect this should be fairly easy to diagnose and resolve :)


IA Page: http://duck.co/ia/view/unicode

marianosimone commented 9 years ago

@moollaza what do you expect it to return? If the same result as "unicode girl" is expected, it seems like, besides Unicode, there's also code for UnicodeFuzzySearch (but I couldn't get any of the test queries in the test file to match

moollaza commented 8 years ago

@marianosimone the Goodie shouldn't return undef if it's bailing out, it should return;

The difference being that return gives an empty list and so we don't recognize the IA as actually triggering or returning an answer. In this case we're seeing that it's actually providing an answer, but the value is simply undef:

1____projects_zeroclickinfo-goodies__perl_

So if there was a test for this query, 'unicode girls' => undef, it would currently be failing:

1____projects_zeroclickinfo-goodies__bash_

/cc @zachthompson do you think we should also handle this in DuckDuckGo/DuckDuckGo?

marianosimone commented 8 years ago

@moollaza I'm even more confused now :confused: If I add that test to t/Unicode.t:

'unicode girls' => test_zci(undef)

It passes without trouble. What's more, I don't see any path where lib/DDG/Goodie/Unicode.pm returns undef while bailing out (but standard returns)

I don't know where the answers for this are coming from, but lib/DDG/Goodie/Unicode.pm doesn't seem to be the one replying (I've even removed that file and run the tests again, and they still pass Note to self: tests need to be run using the -Ilib param

mintsoft commented 8 years ago

@marianosimone I think, it must be section: https://github.com/duckduckgo/zeroclickinfo-goodies/blob/master/lib/DDG/Goodie/Unicode.pm#L153 if unicode_lookup returns undef that'll get returned out by the goodie. The if structure doesn't have an else, so if it doesn't match any of them $result will be undef when it's returned

moollaza commented 8 years ago

@marianosimone there's a big difference between

'unicode girls' => test_zci(undef)

and

'unicode girls' => undef

The former ensures that the result from the IA is actually an ZCI Answer Object, that has the answer property set to undef -- a totally legal, but semantically useless result

The latter ensures that the result from running the handle function is actually undef which is equal to {} (an empty list)

Some details can be found here: http://stackoverflow.com/questions/3435122/whats-the-difference-between-return-and-return-undef-in-perl

The other thing to note is that we expect handle to return an array.

so when you return undef this code: http://stackoverflow.com/questions/3435122/whats-the-difference-between-return-and-return-undef-in-perl

actually gets a valid, 1-element array back, where the value of $results[0] === undef

however when you return in the handle, it actually passes along {} so @array === undef

Does that make sense?

We should really be checking in duckduckgo/duckduckgo if @results is a single-element array containing undef and bail out.

I don't see any reason why we'd need undef to be a valid return value

moollaza commented 8 years ago

@marianosimone it looks like this is the culprit: https://github.com/duckduckgo/zeroclickinfo-goodies/blob/master/lib/DDG/Goodie/Unicode.pm#L44

We return whatever the function returns, and the function unicode_lookup will always return, meaning it can return undef when none of the if blocks execute.

We need a check there, like

return unless my $result = unicode_lookup($1);
return $result;