Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

search, xkcd: built-in search plugin has external replacement #2613

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ classifiers = [
]
requires-python = ">=3.8"
dependencies = [
"xmltodict>=0.12,<0.14",
"pytz",
"requests>=2.24.0,<3.0.0",
"dnspython<3.0",
Expand Down
232 changes: 0 additions & 232 deletions sopel/builtins/search.py

This file was deleted.

77 changes: 37 additions & 40 deletions sopel/builtins/xkcd.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import requests

from sopel import plugin
from sopel.builtins.search import duck_search


LOGGER = logging.getLogger(__name__)
PLUGIN_OUTPUT_PREFIX = '[xkcd] '
Expand All @@ -26,19 +26,21 @@
# https://twitter.com/Dmdboi/status/1589202274999767041
SEARCHXKCD_API = 'https://gq67pznq1k.execute-api.eu-west-1.amazonaws.com/search'

ignored_sites = [
# For searching the web
'almamater.xkcd.com',
'blog.xkcd.com',
'blag.xkcd.com',
'forums.xkcd.com',
'fora.xkcd.com',
'forums3.xkcd.com',
'store.xkcd.com',
'wiki.xkcd.com',
'what-if.xkcd.com',
]
sites_query = ' site:xkcd.com -site:' + ' -site:'.join(ignored_sites)

class SearchXkcdError(Exception):
"""Generic exception to raise if there was a problem contacting the API."""


class SearchFailedError(SearchXkcdError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: That seems a little specialized to me, I would probably have used the base exception to cover this.

Copy link
Member Author

Choose a reason for hiding this comment

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

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".

"""Couldn't reach the search endpoint."""


class ResponseFormatError(SearchXkcdError):
"""Response format couldn't be parsed."""


class NoResultsError(SearchXkcdError):
"""Response could be parsed, but it was empty."""


def get_info(number=None):
Expand All @@ -51,16 +53,6 @@ def get_info(number=None):
return data


def web_search(query):
url = duck_search(query + sites_query)
if not url:
return None
match = re.match(r'(?:https?://)?(?:m\.)?xkcd\.com/(\d+)/?', url)
if match:
return match.group(1)
return None


def searchxkcd_search(query):
parameters = {
'q': query,
Expand All @@ -70,19 +62,20 @@ def searchxkcd_search(query):
response = requests.post(SEARCHXKCD_API, params=parameters)
except requests.exceptions.ConnectionError as e:
LOGGER.debug("Unable to reach searchxkcd API: %s", e)
return None
raise SearchFailedError(str(e))
except Exception as e:
LOGGER.debug("Unexpected error calling searchxkcd API: %s", e)
return None
raise SearchXkcdError(str(e))

try:
hits = response.json()['results']['hits']
if not hits:
return None
raise NoResultsError
first = hits[0]['objectID']
except (JSONDecodeError, LookupError):
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I would have expected a warning here

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

raise ResponseFormatError(msg)

return first

Expand All @@ -98,22 +91,23 @@ def xkcd(bot, trigger):

* If no input is provided it will return a random comic
* If numeric input is provided it will return that comic, or the
nth-latest comic if the number is non-positive
nth-latest comic if the number is negative
* If non-numeric input is provided it will return the first search result
for those keywords on the xkcd.com site
for those keywords from searchxkcd.com

"""
# get latest comic for rand function and numeric input
# get latest comic, for random selection and validating numeric input
latest = get_info()
max_int = latest['num']

# if no input is given (pre - lior's edits code)
if not trigger.group(2): # get rand comic
if not trigger.group(2): # get random comic
dgw marked this conversation as resolved.
Show resolved Hide resolved
random.seed()
requested = get_info(random.randint(1, max_int + 1))
else:
query = trigger.group(2).strip()

numbered = re.match(r"^(#|\+|-)?(\d+)$", query)

if numbered:
query = int(numbered.group(2))
if numbered.group(1) == "-":
Expand All @@ -124,13 +118,16 @@ def xkcd(bot, trigger):
if (query.lower() == "latest" or query.lower() == "newest"):
requested = latest
else:
number = searchxkcd_search(query)
if number is None:
# generic web-search engine as fallback
number = web_search(query)
if not number:
bot.reply('Could not find any comics for that query.')
try:
number = searchxkcd_search(query)
except NoResultsError:
bot.reply("Sorry, I couldn't find any comics for that query.")
return
except (ResponseFormatError, SearchFailedError, SearchXkcdError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Why catch the subclasses here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 😹

bot.reply(
"A technical problem prevented me from searching. "
"Please ask my owner to check my logs.")

requested = get_info(number)

say_result(bot, requested)
Expand Down
Loading
Loading