Skip to content
This repository has been archived by the owner on Oct 15, 2022. It is now read-only.

Switch over spice plugins with regex to trigger words (where applicable) #127

Closed
jagtalon opened this issue Feb 6, 2013 · 10 comments
Closed

Comments

@jagtalon
Copy link
Member

jagtalon commented Feb 6, 2013

Plugins that use trigger words are significantly more efficient. We should really move over as many "regex" plugins as possible (especially those that have use a specific trigger word) to rather trigger initially on that word and in the handle statement use a regex to disqualify any inappropriate searches.

@moollaza
Copy link
Member

moollaza commented Feb 6, 2013

Examples:

triggers query_lc => qr/My spice (.+)/

should really be:

triggers startend => "my spice";

handle remainder => sub {
    ...
}

Also,

triggers query_lc => qr/phone number <specific regex for detecting any phone number>/;

should really be:

triggers startend => "phone number";

handle remainder => sub {

    if (m/<specific regex for detecting any phone number>/) {
        ...
    }else{
        return;
    }
}

@moollaza
Copy link
Member

Here's a list of the current Spice using regex triggers:

$ grep -irn "triggers query_lc" .
./lib/DDG/Spice/Airlines.pm:21:triggers query_lc => qr/^(\d+)\s*(.*?)(?:[ ]air.*?)?$|^(.*?)(?:[ ]air.*?)?\s*(\d+)$/;
./lib/DDG/Spice/IsItUp.pm:15:triggers query_lc => qr/^((?:is\s|))(?:http:\/\/)?([0-9a-z\-]+(?:\.[0-9a-z\-]+)*?)(?:(\.[a-z]{2,4})|)\s(?:up|down|working)/i;
./lib/DDG/Spice/Lastfm/Album.pm:20:triggers query_lc => qr/^([^\s]+(?:\s+[^\s]+)*)\s+(?:albums?|records?|cds?)\s+(?:by|from)?\s+([^\s]+(?:\s+[^\s]+)*)$/;
./lib/DDG/Spice/Lastfm/ArtistAlbum.pm:20:triggers query_lc => qr/^(?:$synonyms)\s+(?:(?:by|from|of)\s+)?([^\s]+(?:\s+[^\s]+)*)$
./lib/DDG/Spice/Lastfm/ArtistTracks.pm:22:triggers query_lc => qr/^(?:(?:all|the)\s+)?(?:$synonyms)\s+(?:(?:by|from|of)\s+)?([^\s]+(?:\s+[^\s]+)*)$
./lib/DDG/Spice/Lastfm/Song.pm:20:triggers query_lc => qr/ ^
./lib/DDG/Spice/Lastfm/TopTracks.pm:20:triggers query_lc => qr/(?:tops?|popular)\s+(?:10\s)?(?:tracks?|charts?|songs?|musics?)(?:\s+in\s+(?:the\s+)?)?(.*)/;
./lib/DDG/Spice/OpenSNP.pm:18:triggers query_lc  => qr/(^rs[0-9]+)$/;
./lib/DDG/Spice/RandNums.pm:21:triggers query_lc => qr/^(?:rand|random) (?:numbers?|nums?)(?: (\-?[0-9]+)\s*(?:-|to)?\s*(\-?[0-9]+))?$/;
./lib/DDG/Spice/RedditSubSearch.pm:18:triggers query_lc => qr#^(?:subreddit|/?r/)\s*(\w+)$|^(\w+)\s+subreddit$#i;
./lib/DDG/Spice/Zipcode.pm:23:triggers query_lc => qr/(?:$zip_string|[a-z\d\-\s]{2,15})/;

@killerfish
Copy link
Contributor

Can we remove RandNums, its using wrap_string_callback => 1; and, is also not in production. The api supports only POST, best if we can make a goodie for this.

@moollaza
Copy link
Member

@killerfish sounds good, go ahead and make a PR to remove it.

@killerfish
Copy link
Contributor

i looked at the rest, 2/3 doable, maybe, but i dont know what would be the threshold for tradeoff, on speed vs. the times its going to trigger (the words are relatively commonly used in queries).

@moollaza
Copy link
Member

@killerfish its definitely beneficial to the system if we can reduce the number of IA's using regex triggers.
Looking again at that list, Airlines, ZipCode and probably OpenSNP should stay as Regex triggers

@moollaza
Copy link
Member

LastFM, Reddit and IsItUp can be switched though

@killerfish
Copy link
Contributor

okay so for reddit, something like this wont work with word triggers, and for isitup, if we consider up|down|working|online|status (from regex) as triggers end, we wont be able to acommodate the random amount of question marks put in the query (which we support currently) so, is http://ddg.gg up would work but is http://ddg.gg up???? wont.

@killerfish
Copy link
Contributor

ill look into LastFM

@moollaza
Copy link
Member

moollaza commented Oct 6, 2014

@killerfish good point about reddit, looks like that'll have to stay. The IsItUp rationale sounds like it's actually a bug though, I think we should be sepearting the question marks from the alpha characters that preceed them. That would require some work in the duckduckgo repo if you're interested. For now though, understood, we'll leave it as is.

Really appreciate you digging into those to figure out if we can switch them!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants