Cleanup hydroshare provider and stop using urlopen#1393
Draft
yuvipanda wants to merge 4 commits intojupyterhub:mainfrom
Draft
Cleanup hydroshare provider and stop using urlopen#1393yuvipanda wants to merge 4 commits intojupyterhub:mainfrom
yuvipanda wants to merge 4 commits intojupyterhub:mainfrom
Conversation
We were: 1. In some cases, directly using requests 2. In some cases, directly using the stdlib's urlopen 3. In some cases, had a method named urlopen that simply passed things through to requests This is unnecessarily confusing, and seems to primarily be done for the benefit of mocking the network calls. However, as described in the recently merged jupyterhub#1390, I don't think mocking is appropriate here as it means we don't actually catch problems. This PR mostly focuses on getting unifying to only using requests directly with as little indirection as possible. If any tests were directly using mocks here, they will be replaced with something that is testing things more directly as appropriate
- Cleanup how the provider is detected, as we were simply doing a domain check but with many extra steps - Move the tests to be real integration tests - Test detection, not content_id
yuvipanda
force-pushed
the
only-requests
branch
from
b8b7fc6 to
cd9911c
Compare
for more information, see https://pre-commit.ci
yuvipanda
changed the title
32 tasks
Contributor
|
@yuvipanda what is the status of this? #1488 was also related with hydroshare. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sort-of a follow up to #993.
Follow-up to #1390, in moving hydroshare tests to be integration tests
It does two things, that are intertwined:
Cleanup using
urlopenvsrequestsWe were:
This is unnecessarily confusing, and seems to primarily be done for the benefit of mocking the network calls. However, as described in the recently merged #1390, I don't think mocking is appropriate here as it means we don't actually catch problems.
This PR mostly focuses on getting unifying to only using requests directly with as little indirection as possible.
Cleanup hydroshare
The biggest user of the slightly easier mocking provided by the
urlredirection was hydroshare. Similar to #1390, this PR now: