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

HTTP Requests response status code handlings #42

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Xlitoni
Copy link
Collaborator

@Xlitoni Xlitoni commented Nov 16, 2023

The goal of this PR is to close #41 & close #28.

This aims to make the bot handle more HTTP responde status codes, 40X errors that mean there is a problem with the request and 50X errors that means there is a problem on server side.

@Xlitoni Xlitoni self-assigned this Nov 16, 2023
Copy link
Contributor

@ctmbl ctmbl left a comment

Choose a reason for hiding this comment

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

a first review about new RateLimiter exceptions

Comment on lines +25 to +31
class RLRequestFailed(RateLimiterError):
def __init__(self, request, *args, **kwargs):
super().__init__(request, *args, **kwargs)

class RLRequestClientError(RLRequestFailed):
def __init__(self, request, *args, **kwargs):
super().__init__(request, *args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe RLClientError, RLClientError4xx would be a better name?
(same with Server)

Comment on lines +29 to +31
class RLRequestClientError(RLRequestFailed):
def __init__(self, request, *args, **kwargs):
super().__init__(request, *args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe RLClientError, RLClientError4xx would be a better name?
(same with Server)

Copy link
Contributor

Choose a reason for hiding this comment

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

also the name RLRequestServerError wrongly give the impression of inheriting from RLRequestFailed

Comment on lines +25 to +27
class RLRequestFailed(RateLimiterError):
def __init__(self, request, *args, **kwargs):
super().__init__(request, *args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken the whole point of this exception is to allow future subclassing (because today only RLRequestClientError inherits from it)
However it made the code a bit more obscure (especially because you except the parent class not this one)
So in the interest of readability maybe we could get rid of this one and instead except RLClientError later?

Xlitoni added 2 commits October 27, 2024 19:34
…, server error mean longer time to wait, client error means request failed
@ctmbl ctmbl force-pushed the request-response-code-handlings branch from b284a6d to 4b72f13 Compare October 27, 2024 18:58
@ctmbl ctmbl force-pushed the request-response-code-handlings branch from 4b72f13 to 9121289 Compare October 27, 2024 18:59
@ctmbl ctmbl mentioned this pull request Oct 28, 2024
4 tasks
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.

Handle 401 error Handle 404 error
2 participants