pyrollbar icon indicating copy to clipboard operation
pyrollbar copied to clipboard

Move requests package to optional dependency

Open phillipuniverse opened this issue 3 years ago • 7 comments

I have a fully asynchronous FastAPI application that I use with Rollbar for error tracking. I want to avoid other developers accidentally utilizing the requests package to make external API calls as it will introduce blocking code within a fully asynchronous service.

However, requests is non-optional https://github.com/rollbar/pyrollbar/blob/55d08a65c626539d5a74a0439facc992514247e4/setup.py#L82-L87 and will always come in as a transitive dependency with Rollbar.

Ideally, adding a dependency on pyrollbar would not transitively bring in the requests package.

phillipuniverse avatar Nov 29 '22 04:11 phillipuniverse

Thank you @phillipuniverse for bringing this up. That makes a lot of sense!

We currently import requests at the package level and use it as the default HTTP sender for a few handlers. We will need to try to import it if the handler uses it and throw an exception if it does not exist.

Because of that, this is a breaking change. I am adding this to our v1.0.0 milestone. We were talking internally about getting to v1.0.0 yesterday, so hopefully this is not too far in the future.

Is this update something you would be willing to contribute to?

danielmorell avatar Nov 29 '22 16:11 danielmorell

@danielmorell yes, I can work on a contribution for this.

Do y'all have a candidate timeline for when you are looking at a 1.0 release?

phillipuniverse avatar Nov 29 '22 16:11 phillipuniverse

Thank you @phillipuniverse. I greatly appreciate that!

I am aiming at release Q1 2023. But I will need to talk to product before anything concrete is determined.

danielmorell avatar Nov 29 '22 16:11 danielmorell

Please also add optional dependencies for other handlers like async. Currently there is a note in documentation:

The following examples use the async handler that requires the HTTPX package to be installed.

This is not the way how dependencies should be listed.

homm avatar Mar 02 '23 08:03 homm

@danielmorell candidate PR at #422.

This is not the way how dependencies should be listed.

@homm indeed, it should be an extras_require, right? I'll wait for further feedback but could definitely roll that into my changes in #422.

phillipuniverse avatar Mar 02 '23 22:03 phillipuniverse

@phillipuniverse Right. I think it's a good idea to define one key in extras_require for every handler (even if the actual list is empty) so the one could specify rollbar[thread]>=0.17.0 and not worry about extra requirements even for future versions.

homm avatar Mar 03 '23 09:03 homm

I decided to go in a slightly different direction here that didn't rely on removing the requests package from pyrollbar. I used flake8-tidy-imports, specifically from within Ruff to ban the requests api in my async projects. Example from my pyproject.toml:

    [tool.ruff.flake8-tidy-imports.banned-api]
    "requests".msg = "Use httpx instead"

This achieves my purposes of trying to prevent developers from using blocking code accidentally.

phillipuniverse avatar Apr 13 '23 20:04 phillipuniverse