-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: add suport for Metadata API #26
base: main
Are you sure you want to change the base?
Conversation
Related: radiorabe/suisa_sendemeldung#268 |
return self.json(path="/api/external-metadata/tracks", params=params).get( | ||
"data" | ||
) |
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.
it should probably also pass kwargs along so users can pass additional args to requests.get
(like a session for retry/backoff).
return self.json(path="/api/external-metadata/tracks", params=params).get( | |
"data" | |
) | |
return self.json( | |
path="/api/external-metadata/tracks", | |
params=params, | |
**kwargs, | |
).get("data") |
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.
Ok good idea
"data" | ||
) | ||
except HTTPError as error: | ||
print(error) |
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.
imo libs should not have side effects like print()
print(error) |
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.
Yes, I just pushed the code to have it somewhere. I agree :)
return self.json(path="/api/external-metadata/tracks", params=params).get( | ||
"data" | ||
) | ||
except HTTPError as error: |
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.
Is this error persistent? if it's a temporary error a retry/backoff loop might be better than ignoring it.
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.
🤔 retry/backoff code is already here:
python-acrclient/acrclient/client.py
Lines 46 to 56 in 399bcc9
Parameters: | |
retries: Total number of retries to allow. | |
backoff_factor: A backoff factor to apply between attempts after the | |
second try (most errors are resolved immediately by a second try | |
without a delay). urllib3 will sleep for:: | |
{backoff factor} * (2 ** ({number of total retries} - 1)) | |
seconds. If the backoff_factor is 0.1, then :func:`Retry.sleep` | |
will sleep for [0.0s, 0.2s, 0.4s, ...] between retries. It will | |
never be longer than `backoff_max`. | |
By default, backoff is set to 0.1. |
maybe we need to tune it, or it doesn't include 500 errors
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.
For some entries, the error persists and for some it returns 200 after retrying a few seconds later. You're absolutely correct to handle this in the central location.
Reference: https://docs.acrcloud.com/reference/metadata-api
This adds support for the Metadata API.