search, xkcd: built-in search plugin has external replacement #2613
Conversation
dgw
added
Low Priority
Housekeeping
dgw
changed the title
|
Staying in draft until type-checking errors are fixed, which should be accomplished by #2614. |
The built-in search plugin is pretty flaky, so it would be dubiously useful at best even if searchxkcd stops working. Plus, I'm working on replacing the built-in search plugin entirely with a more reliable external plugin built on a real library (instead of hacky regex-based web scraping).
Also lets us drop `xmltodict` as a core dependency; `search.py` was the last place using it.
|
Rebased onto |
| """Generic exception to raise if there was a problem contacting the API.""" | ||
|
|
||
|
|
||
| class SearchFailedError(SearchXkcdError): |
There was a problem hiding this comment.
Nitpick: That seems a little specialized to me, I would probably have used the base exception to cover this.
There was a problem hiding this comment.
Maybe it needs a better name? The idea was to shadow requests.exceptions.ConnectionError with a plugin-specific error type—note where SearchFailedError is used below. It's an attempt to differentiate between errors of the "I talked to the API but couldn't understand what it said back" kind and more serious "the API might be gone???" issues.
I might look at this again when I go through to fix up things like your logging comment and reevaluate the exception hierarchy, even though you did say "LGTM".
| LOGGER.debug("Data format from searchxkcd API could not be understood.") | ||
| return None | ||
| msg = "Data format from searchxkcd API could not be understood." | ||
| LOGGER.debug(msg) |
There was a problem hiding this comment.
Huh, I would have expected a warning here
There was a problem hiding this comment.
To be fair I often err on the side of "don't spew stuff into logs if the user isn't actively debugging", but this one doesn't carry a big dump of stuff with it. What do you think of a warning with the user-friendly error message here and then a debug logging call with the API response?
| return | ||
| except (ResponseFormatError, SearchFailedError, SearchXkcdError): |
There was a problem hiding this comment.
Nitpick: Why catch the subclasses here?
There was a problem hiding this comment.
I'm gonna say it's because I did this at some ungodly wee hour and forgot that they were subclasses despite just creating the new exception types mere moments earlier. Possibly also related to the fact that I started out creating an Enum type for the search function to return on errors, before deciding exceptions were better.
tl;dr: I dunno, weird brain stuff. 😹
| return | ||
| except (ResponseFormatError, SearchFailedError, SearchXkcdError): |
There was a problem hiding this comment.
I'm gonna say it's because I did this at some ungodly wee hour and forgot that they were subclasses despite just creating the new exception types mere moments earlier. Possibly also related to the fact that I started out creating an Enum type for the search function to return on errors, before deciding exceptions were better.
tl;dr: I dunno, weird brain stuff. 😹
| """Generic exception to raise if there was a problem contacting the API.""" | ||
|
|
||
|
|
||
| class SearchFailedError(SearchXkcdError): |
There was a problem hiding this comment.
Maybe it needs a better name? The idea was to shadow requests.exceptions.ConnectionError with a plugin-specific error type—note where SearchFailedError is used below. It's an attempt to differentiate between errors of the "I talked to the API but couldn't understand what it said back" kind and more serious "the API might be gone???" issues.
I might look at this again when I go through to fix up things like your logging comment and reevaluate the exception hierarchy, even though you did say "LGTM".
| LOGGER.debug("Data format from searchxkcd API could not be understood.") | ||
| return None | ||
| msg = "Data format from searchxkcd API could not be understood." | ||
| LOGGER.debug(msg) |
There was a problem hiding this comment.
To be fair I often err on the side of "don't spew stuff into logs if the user isn't actively debugging", but this one doesn't carry a big dump of stuff with it. What do you think of a warning with the user-friendly error message here and then a debug logging call with the API response?
Description
The built-in
searchhas been unreliable, often yielding no results because it probably gets blocked from scraping Bing or DuckDuckGo SERPs. (This mechanism isn't entirely clear, as it still does work from some machines—including places like Gitpod cloud development servers that presumably live in a higher-profile public cloud than any of the bots with problems.)However, a near-scratch rewrite at sopel-irc/sopel-search using a library to access DDG seems to be significantly more reliable, and lets us drop
xmltodictas a core dependency;search.pywas the last place using it.This patch also removes
sopel.builtins.search.duck_search()as a fallback from thexkcdplugin, for better or worse putting all of its proverbial eggs in thesearchxkcd.combasket. (I reworked the error-handling there a little, too.)Checklist
make qa(runsmake lintandmake test)Notes
I see no problem with doing this in 8.1, but am open (as always) to other opinions 😸