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

Cloud backup support #33

Merged
merged 4 commits into from
Dec 9, 2024
Merged

Cloud backup support #33

merged 4 commits into from
Dec 9, 2024

Conversation

mdegat01
Copy link
Collaborator

@mdegat01 mdegat01 commented Dec 7, 2024

Proposed Changes

Related Issues

(Github link to related issues or pull requests)

@mdegat01 mdegat01 added the new-feature New features or options. label Dec 7, 2024
@@ -146,7 +155,7 @@ async def get(
self,
uri: str,
*,
params: dict[str, str] | None = None,
params: dict[str, str] | MultiDict[str, str] | None = None,
Copy link
Contributor

@emontnemery emontnemery Dec 7, 2024

Choose a reason for hiding this comment

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

mypy tells me MultiDict takes a single type parameter, the keys are always str

Maybe just type this as aiohttp's Query if aiohasupervisor does not modify it?
https://docs.aiohttp.org/en/stable/client_reference.html#aiohttp.ClientSession.request
https://github.com/aio-libs/aiohttp/blob/dbd77ad6e46a0f89268758cdc06c20afeaf13e96/aiohttp/client.py#L171

Suggested change
params: dict[str, str] | MultiDict[str, str] | None = None,
params: Query = None,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems Query would need a newer aiohttp version, it came in with this PR in 3.11.0 I think (currently pyproject.toml specifies 3.3.0 as minimum).

Let's go with the fixed MultiDict typing for now.

@@ -94,42 +117,28 @@ async def _request(
self._close_session = True

try:
async with self.session.request(
response = await self.session.request(
Copy link
Contributor

@emontnemery emontnemery Dec 7, 2024

Choose a reason for hiding this comment

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

I'm not sure it matters, but if want to keep the context manager we could separate the cases where we can use the context manager from the cases we don't:

if response_type == ResponseType.RAW:
    response = await self.session.request(
        ...
    )
    ...
else:
    async with self.session.request(
        ...
    ) as response:
        ...

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Tested locally, everything works as expected 👍

aiohasupervisor/client.py Outdated Show resolved Hide resolved
aiohasupervisor/client.py Outdated Show resolved Hide resolved
aiohasupervisor/client.py Outdated Show resolved Hide resolved
aiohasupervisor/client.py Outdated Show resolved Hide resolved
aiohasupervisor/client.py Outdated Show resolved Hide resolved
@agners agners requested a review from emontnemery December 9, 2024 10:11
@agners agners marked this pull request as ready for review December 9, 2024 10:12
Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

👍

@agners agners merged commit 202f070 into main Dec 9, 2024
7 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
@agners agners deleted the cloud-backup-support branch January 29, 2025 10:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-feature New features or options.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants