Ruff DOC502 preview false positive with requests.Response.raise_for_status()

[jamesbraza]

With ruff==0.5.7 and turning on the preview rule DOC502/docstring-extraneous-exception (made as part of #12434), I think perhaps requests.Response.raise_for_status() is a special case where we should not alert DOC502 for HTTPError:

import requests

def run_get(url: str) -> None:
    """
    Run a GET request to the given URL.

    Raises:
        requests.exceptions.HTTPError: if the GET request fails.
    """
    response: requests.Response = requests.get(url, timeout=30.0)
    response.raise_for_status()  # This can raise HTTPError

run_get("https://httpbin.org/status/500")

Ruff will print what is imo a false positive:

DOC502 Raised exception is not explicitly raised: `requests.exceptions.HTTPError`

[charliermarsh]

It's a slippery slope to start inferring exception types from arbitrary library code. Perhaps we can cover some of the common cases though.

[jamesbraza]

I completely agree it's a slippery slope haha. It may also be that functions/methods like raise_for_status go against the core of DOC502, so feel free to close this out

[AlexWaygood]

Perhaps we can cover some of the common cases though.

We could also... make it configurable... e.g. you could have a setting where you could configure "Oh, a call to this function always raises X exception", etc.

(Obligatory meme)

[MichaReiser]

Adding configuration solves a different need. It allows you to make Ruff behave correctly for your user code or a library that isn't commonly used. I think it would be somewhat annoying if every requests users has to write the same configuration.

[AlexWaygood]

I think it would be somewhat annoying if every requests users has to write the same configuration.

Maybe. We have to draw the line somewhere, though, and "we will attempt to handle stdlib functions but not arbitrary third-party functions that raise exceptions" feels like the logical line for us to draw for me.

[RubenVanEldik]

I also just ran into this error!

To play devil's advocate. I think this slope is not super slippery, since there aren't that many methods in popular libraries that have the sole purpose of raising an error (afaik).

I think expanding this scope to include the raise_for_status methods of the more popular request libraries (requests, httpx, aiohttp, and urlllib3) would solve this issue for >90%, without putting the scope of this rule on an ever increasing path.

I would to be proven wrong. If there are many more methods in popular libraries that have the sole purpose of raising an exception, please let me know!

[tjkuson]

We could also... make it configurable... e.g. you could have a setting where you could configure "Oh, a call to this function always raises X exception", etc.

Would users necessarily need to encode the exception types as well? That seems a bit overkill. It could be a list of APIs that are known to return typing.Never (with no consideration for what the function does) that's eventually phased out by red-knot. If the function body were to contain a call to a typing.Never object, skip this rule. It could be a higher level configuration too with other checks using it (off the top of my head, dead code analysis).

[RubenVanEldik]

I think it would be useful, as response.raise_for_status() is basically shorthand for:

if not response.ok:
    raise requests.exceptions.HTTPError(...)

It's just a lazy way to raise an error, so I think it makes sense to include that exception in the docstring.

[tjkuson]

Oh, so the return type for requests.Response.raise_for_status is None? That complicates things. My intuition is that it doesn't seem to be in the spirit of the diagnostic to permit that…

Edit: I should have spotted that in original post, sorry.