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

Do not force the requests module to always be included #422

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phillipuniverse
Copy link

Description of the change

Allows the module to work if requests is not available.

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance
  • New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers assigned
  • Issue from task tracker has a link to this pull request
  • Changes have been reviewed by at least one other engineer

@phillipuniverse phillipuniverse marked this pull request as draft March 2, 2023 22:05
@phillipuniverse
Copy link
Author

I really just wanted to get this out quickly to see what CI would say, I'm not very confidant in my changes there. I was also surprised that all of the tests passed locally with no other changes but maybe that's a good thing?

I tried to make as small and as targeted of a change as possible but it's tricky, there is a lot of "hand shake" type of agreements. I don't have a ton of confidence that a user wouldn't get into a code path that refers to the requests module.

But I suppose if all the tests pass in the FastAPI/Starlette environments that doesn't have requests installed then maybe we're in the clear.

@phillipuniverse
Copy link
Author

@danielmorell could I get some feedback on my solution here? Is this in the right direction?

@danielmorell
Copy link
Collaborator

Hey @phillipuniverse, sorry for the long wait! Overall, I think this is moving in the right direction, and solving a very real problem. Thank you for your work on this!

It may make sense to check if requests is available when Rollbar is initialized instead of waiting until we try to process the first error message. I feel like letting the developer know as soon as possible that they have configured a state that won't work would be best.

What do you think?

@danielmorell danielmorell self-assigned this Apr 28, 2023
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 this pull request may close these issues.

Move requests package to optional dependency
2 participants