juledwar / soufi

Source finder CLI and API
Apache License 2.0
0 stars 0 forks source link

Add a status_code attribute to DownloadError exceptions #28

Closed 0xDEC0DE closed 2 years ago

0xDEC0DE commented 2 years ago

One of the more common causes of DownloadError exceptions in the wild is HTTPError exceptions.

Surface the status code from the HTTP response as a convenience to callers.

Drive-by: update the version of Black used for tests, for compatibility with modern Click.

Fixes Issue #27


This change is Reviewable

0xDEC0DE commented 2 years ago

This looks and feels like a hack. I will think about it some more because we need a consistent finder experience, and some are raising DownloadError with strings, while the base class's make_archive is using the request's Response.

I have a lot of rationales packed into a very confined space, and explaining them all might turn into a self-contradictory rant on my part, so I'll simply state:

The motivation behind it all is, "libraries should be helpful".

I wanted to keep it all as a single exception class, rather than a rich set of exceptions based on what was throwing it, because I find exceptions that follow the "junk-drawer" pattern to be the most broadly useful.

I'm similarly averse to trying to make the library have an opinion about whether it was thrown with a string object, Response object, raise from, or whatever.

I'm open to doing this instead as a more robust/rich set of exceptions, and some added rigor about how they are thrown, but I don't really see it as having much benefit over providing a single exception class, with a "stable" interface for callers to use when handling it.

juledwar commented 2 years ago

I have a lot of rationales packed into a very confined space, and explaining them all might turn into a self-contradictory rant on my part, so I'll simply state:

The motivation behind it all is, "libraries should be helpful".

We are in violent agreement!

I wanted to keep it all as a single exception class, rather than a rich set of exceptions based on what was throwing it, because I find exceptions that follow the "junk-drawer" pattern to be the most broadly useful.

I'm similarly averse to trying to make the library have an opinion about whether it was thrown with a string object, Response object, raise from, or whatever.

I'm open to doing this instead as a more robust/rich set of exceptions, and some added rigor about how they are thrown, but I don't really see it as having much benefit over providing a single exception class, with a "stable" interface for callers to use when handling it.

What I had in mind was a constructor on the DownloadError exception that explicitly takes a limited set of options, so that API users have a reasonable expectation of what it might look like. At the moment, it's a placeholder, or the "junk-drawer" as you succinctly put it.

Instead I'd like it to be constructable with a string arg and an optional Response object. The API into the exception can then surface:

  1. The original string arg (which is the error text)
  2. Any optional status code (taken from the Response if it exists, similar to how you're doing it now but in a more strict way). It can be None if there's no Response.
  3. The raw Response, if one exists

This will let call sites avoid iterating over potential multiple exception args (which is just stupid) and use the DownloadError's methods with confidence.

So something like this:

class DownloadError(Exception):
    def __init__(self, error_text, response=None):
        super().__init__(error_text)
        self.response = response

    @property
    def status_code(self):
        if self.response is None:
            return None
        return self.response.status_code

The caller can do both str(e) to get the error text, AND inspect the status_code / response as it wants. Obviously this should all be documented in the docstring.

We would also need to evaluate the existing call sites. I think this is backwardly compatible with everything, as I originally envisaged a minor version bump, which might not be needed.

0xDEC0DE commented 2 years ago

I can get on board with this. Let me re-work this a bit to get everybody in alignment.