-
Notifications
You must be signed in to change notification settings - Fork 39
Fromager 361: Requsts Session Object #372
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
Conversation
shubhbapna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing it in context, should we create a module instead that exposes this session object. Then instead of passing the context everywhere, we just import the session object?
I had tried separate module few weeks ago in #228 |
Oh is this the module: 4cc20c8 ? The module in this commit defines a class but there is no object that can be used directly on importing. I was thinking something along these lines:
import request
session = request.session()Then say in from .request_session import session
# the function signature doesn't change at all
def get_project_from_pypi(
project: str,
extras: typing.Iterable[str],
sdist_server_url: str,
) -> typing.Iterable[Candidate]:
"""Return candidates created from the project name and extras."""
simple_index_url = sdist_server_url.rstrip("/") + "/" + project + "/"
logger.debug("%s: getting available versions from %s", project, simple_index_url)
# using the session object directly
data = session.get(simple_index_url).content |
yeah, that's the one. |
Done |
dhellmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits, but this generally looks good.
|
I'm happy with this version. @shubhbapna had comments, so I'll leave it open for him to review. I re-ran the coverage job that failed and it failed again. Is that a known problem? |
I am not sure. I am seeing it for the first time |
shubhbapna
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I reran the checks in hopes of regenerating the coverage files. The coverage job was failing because no artifacts were found for some reason
Fixes: #361