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

fix aiohttp redirect #126

Merged
merged 5 commits into from
Mar 24, 2023
Merged

fix aiohttp redirect #126

merged 5 commits into from
Mar 24, 2023

Conversation

dreamflasher
Copy link
Contributor

@dreamflasher dreamflasher commented Jan 19, 2023

Closes #125

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Base: 89.87% // Head: 89.87% // No change to project coverage 👍

Coverage data is based on head (77d0810) compared to base (2aa5790).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 77d0810 differs from pull request most recent head 34fb61a. Consider uploading reports for the commit 34fb61a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #126   +/-   ##
=======================================
  Coverage   89.87%   89.87%           
=======================================
  Files           5        5           
  Lines         622      622           
=======================================
  Hits          559      559           
  Misses         63       63           
Impacted Files Coverage Δ
parfive/downloader.py 89.10% <ø> (ø)
parfive/config.py 97.77% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dreamflasher
Copy link
Contributor Author

making the Optional explicit will also allow to auto-merge this: #118

@@ -35,7 +35,7 @@ def _default_aiohttp_session(config: "SessionConfig") -> aiohttp.ClientSession:
`aiohttp.ClientSession` expects to be instantiated in a asyncio context
where it can get a running loop.
"""
return aiohttp.ClientSession(headers=config.headers)
return aiohttp.ClientSession(headers=config.headers, requote_redirect_url=False)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a minimum version of aiohttp just to be safe (tbh we should already have one and I don't know if this is actually the thing that constrains us).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, but you do not have any versioning of dependencies in this project, yet. So for this PR I'd suggest to continue without. And in parallel, what do you think about migrating this project to poetry? It's pretty straightforward, and I am happy to help with a PR.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather stick to setuptools for now, although I would be happy to move all the config into pyproject.toml.

Comment on lines +154 to +156
filename: Optional[
Union[str, Callable[[str, Optional[aiohttp.ClientResponse]], os.PathLike]]
] = None,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I think type hints have gone too far 😛

@Cadair Cadair merged commit eb4438b into Cadair:main Mar 24, 2023
@Cadair Cadair added the bug A report of or a fix for a bug or unwanted behaviour label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A report of or a fix for a bug or unwanted behaviour
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Redirects are not resolved
2 participants