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

Removed vendored requests and certifi that are unused. #5001

Closed
wants to merge 7 commits into from

Conversation

matteius
Copy link
Member

@matteius matteius commented Mar 22, 2022

Removes vendored requests and certifi because we can -- its not what we use in the project.

Edit: so it appears we do use it, not sure why the coverage report has mislead me here. Will revisit.

The issue

#5000

@matteius matteius added the Status: Deferred / On Hold 🛑 This item is on hold until further notice. label Mar 22, 2022
@matteius matteius changed the title Remoevd vendored requests and certifi that are unused. Removed vendored requests and certifi that are unused. Mar 22, 2022
@frostming
Copy link
Contributor

Coverage report only tells there are not tests for it, rather than it isn't used. Using the coverage report as a mark may be misleading.

@matteius
Copy link
Member Author

Coverage report only tells there are not tests for it, rather than it isn't used. Using the coverage report as a mark may be misleading.

Totally agree with the premise of your comment--in this case it is code that was exercised by tests but was showing red on the coverage report which seems odd. At first I suspect that maybe because of where the import was happening or maybe it was using the requests from the Pipfile.lock requirements, but really if it was it wouldn't have blown up pulling out the vendor version.

I was thinking there could be some value in being consistent in using the same version of requests for both pip and the remainder of the project but also still exploring this one. @frostming What are your thoughts about relying on the requests provided by pip rather than explicitly vendoring our own?

@matteius
Copy link
Member Author

I'll revisit this in a fresh PR.

@matteius matteius closed this Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Deferred / On Hold 🛑 This item is on hold until further notice.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants