-
Notifications
You must be signed in to change notification settings - Fork 51
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
Allow finer control of API errors in client code (issue #43) #44
base: master
Are you sure you want to change the base?
Conversation
According to https://www.mediawiki.org/wiki/Manual:Maxlag_parameter, this may happen for caching layer errors.
Also remove some retry logic, since we now retry from query().
|
||
|
||
retries - How many retries to allow for in case of errors (set to 0 for no | ||
retries). If the number of attempts is exceeeded, an exception is raised: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exceeeded"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
except Exception, e: | ||
print 'Attempt %d (of %d) raised exception %r' % (attempt, attempts, e) | ||
if attempt == attempts: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what do you suggest?
@@ -317,7 +317,8 @@ def __exceptionFromAPIError(self, data, content): | |||
raise APIError(data) | |||
error = content['error']['code'] | |||
if error == "maxlag": | |||
lagtime = int(re.search("(\d+) seconds", content['error']['info']).group(1)) | |||
lagtime = 0.5 + float(re.search("([0-9.-]+) seconds", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this addition of "0.5" have an inline code comment? It seems non-obvious to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, never mind. This isn't really new logic.
How about something like this for #43? This also includes a little bit of cleanup, and I think it's ready for a first look.
I've only tested manually so far (basically following the examples in the README with different retry parameters). I think I might be able to add some unit tests later, but let me know if there are any other testing you normally do.