GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||
GeezerVison is showing github.com — [DEV-12952] Add Configurable Retry Behavior by mawelborn · Pull Request #350 · IndicoDataSolutions/indico-client-python Conversation
|
|
Hi @IndicoDataSolutions/pr-be-indicodata-ai folks! I rebased this PR onto main and have all the checks passing. Would appreciate a review when you have a chance. This functionality will be super useful for ProServe and customers. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
if this is an urgent thing, this can probably wait, but on the platform side we've been converging on this library, which might be worth considering here instead of shipping our own? it has fancy timing, logging, and backoff stuff we could probably expose if we wanted for better script integrations
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hello again! Sorry, just now getting back to this. I don't have strong opinions regarding the retry implementation--whether we ship our own or use a library like Tenacity. I used the retry implementation from the toolkit since it's a known quantity, and shipping our own was already being done for AsyncIndicoClient. Do I need to swap out the implementation or make any other changes? Or is the current implementation good enough for now? Thanks! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| """ | ||
| def retry_decorator( | ||
| decorated: "Callable[ArgumentsType, InnerReturnType]", | ||
| ) -> "Callable[ArgumentsType, Awaitable[InnerReturnType]] | Callable[ArgumentsType, InnerReturnType]": # noqa: E501 |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't need both inner and outer return type here i don't think? both the wrapper and the fn should be typed to return the same thing (since they do). that would probably alleviate the any typing problem too
also | isn't valid for <3.10. has to be Union[]
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch! Those two return types are the same in this version.
It's valid on 3.9 when string escaped, like the other types that are only imported when type checking. I've run it and type checked it with mypy --strict on 3.9 without issue.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@IndicoDataSolutions/pr-be-indicodata-ai it looks like one of you will have to approve and/or merge? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.