Do not force the requests module to always be included

phillipuniverse commented 1 year ago

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
[x] New release

Related issues

Shortcut stories and GitHub issues (delete irrelevant)

Fix [SC-]
Fix #417

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 commented 1 year ago

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 commented 1 year ago

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

danielmorell commented 1 year ago

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?

rollbar / pyrollbar