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

Move requests package to optional dependency #417

Open
phillipuniverse opened this issue Nov 29, 2022 · 7 comments · May be fixed by #422
Open

Move requests package to optional dependency #417

phillipuniverse opened this issue Nov 29, 2022 · 7 comments · May be fixed by #422
Assignees
Milestone

Comments

@phillipuniverse
Copy link

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

pyrollbar/setup.py

Lines 82 to 87 in 55d08a6

# In the current version, `requests>= 0.12.1`
# always installs the latest version of the package.
'requests>=0.12.1; python_version == "2.7"',
'requests>=0.12.1; python_version >= "3.6"',
'requests<2.26,>=0.12.1; python_version == "3.5"',
'requests<2.22,>=0.12.1; python_version == "3.4"',
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.

@danielmorell danielmorell self-assigned this Nov 29, 2022
@danielmorell danielmorell added this to the v1.0.0 milestone Nov 29, 2022
@danielmorell
Copy link
Collaborator

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?

@phillipuniverse
Copy link
Author

@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?

@danielmorell
Copy link
Collaborator

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.

@homm
Copy link
Contributor

homm commented Mar 2, 2023

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.

phillipuniverse added a commit to phillipuniverse/pyrollbar that referenced this issue Mar 2, 2023
@phillipuniverse phillipuniverse linked a pull request Mar 2, 2023 that will close this issue
12 tasks
@phillipuniverse
Copy link
Author

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

@homm
Copy link
Contributor

homm commented Mar 3, 2023

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

@phillipuniverse
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants