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

On predicate #70

Merged
merged 12 commits into from
Aug 5, 2024
Merged

On predicate #70

merged 12 commits into from
Aug 5, 2024

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Jul 21, 2024

@hynek
Copy link
Owner

hynek commented Jul 22, 2024

@dhirschfeld @estahn Before I dive into this1 – opinions?

Footnotes

  1. I really need to get out attrs 24.1 before starting something new 🫠

Comment on lines 64 to 66
On = Union[
Type[Exception], Tuple[Type[Exception], ...], Callable[[Exception], bool]
]

Choose a reason for hiding this comment

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

I'm not a big fan of the name. on: On doesn't convey any meaning to the reader of the code.

My own inclination would be to try and give some meaning to the name - e.g. on: exc_or_predicate. It's not perfect, but I think it's probably better than conveying no meaning in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to ExcOrPredicate

Copy link

@dhirschfeld dhirschfeld left a comment

Choose a reason for hiding this comment

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

Just some stylistic comments which @hynek can decide upon, otherwise I think it looks good.

@dhirschfeld
Copy link

Resolves #63

@estahn
Copy link

estahn commented Aug 5, 2024

Does this need documentation or an example for the docs?

@hynek
Copy link
Owner

hynek commented Aug 5, 2024

Does this need documentation or an example for the docs?

impeccable timing :D

@hynek
Copy link
Owner

hynek commented Aug 5, 2024

I've just added the docs and am heading to the gym. If y'all could have look at them until I'm back that would be appreciated: https://py-stamina--70.org.readthedocs.build/en/70/

@estahn
Copy link

estahn commented Aug 5, 2024

I've just added the docs and am heading to the gym. If y'all could have look at them until I'm back that would be appreciated: https://py-stamina--70.org.readthedocs.build/en/70/

Looks good to me, although I realise it doesn't fully support my use-case I think.

We integrate with a large number of different API vendors and each have their own quirks. One of them being the handling of quota's. Some of those API's will respond with a 429 and provide quota details like time to next retry. I figured I could use this information as the basis for the retry mechanics, but wait is not a callable.

Similar to this example.
https://www.seelk.co/blog/efficient-client-side-handling-of-api-throttling-in-python-with-tenacity/

As for this PR 👍🏼 🚀

@hynek
Copy link
Owner

hynek commented Aug 5, 2024

I realise it doesn't fully support my use-case I think.

We integrate with a large number of different API vendors and each have their own quirks. One of them being the handling of quota's. Some of those API's will respond with a 429 and provide quota details like time to next retry. I figured I could use this information as the basis for the retry mechanics, but wait is not a callable.

So what you want is being about to return a float that means "yes, but at least this long"?

Feel free to open a discussion about it, ideally with a simplified requirement vs whole blog post. 😇

As for this PR 👍🏼 🚀

Thanks!

@hynek hynek merged commit a5c37a8 into hynek:main Aug 5, 2024
21 checks passed
@estahn
Copy link

estahn commented Aug 13, 2024

@hynek Thanks for merging. Is there anything else required before releasing this?

@hynek
Copy link
Owner

hynek commented Aug 14, 2024

yes, since I work in batches, I want to tackle #71 and #59 before releasing and moving on (to svcs).

@gsakkis gsakkis deleted the on-predicate branch August 14, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants