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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add suport for Metadata API #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion acrclient/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
from requests import Session
from requests.adapters import HTTPAdapter, Retry
from requests.auth import AuthBase
from requests.exceptions import HTTPError
from requests.models import PreparedRequest, Response

from .models import GetBmCsProjectsResultsParams
from .models import GetBmCsProjectsResultsParams, GetExternalMetadataTracksParams


class _Auth(AuthBase): # pylint: disable=too-few-public-methods
Expand Down Expand Up @@ -155,3 +156,14 @@ def get_bm_cs_projects_results(
params=params,
**kwargs,
).get("data")

def get_external_metadata_tracks(
self, params: Optional[GetExternalMetadataTracksParams] = None
) -> list:
try:
return self.json(path="/api/external-metadata/tracks", params=params).get(
"data"
)
Comment on lines +164 to +166
Copy link
Member

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).

Suggested change
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")

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok good idea

except HTTPError as error:
Copy link
Member

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.

Copy link
Member

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

Copy link
Member Author

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.

print(error)
Copy link
Member

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()

Suggested change
print(error)

Copy link
Member Author

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 []
13 changes: 12 additions & 1 deletion acrclient/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import TypedDict
from typing import Literal, TypedDict


class GetBmCsProjectsResultsParams(TypedDict):
Expand All @@ -14,3 +14,14 @@ class GetBmCsProjectsResultsParams(TypedDict):
"""Return the results with played_duration <= max_duration seconds (default: 3600)"""
isrc_country: str
"""Only return results that match the isrc country code (E.g. DE, FR, IT, US)"""


class GetExternalMetadataTracksParams(TypedDict):
"""Parameters for getting music platforms metadata and links"""

type: str
"""ACRCloud Music ID"""
acr_id: str
"""1 or 0, if you set it to 1, the response metadata will contain the contributors and works
metadata if the tracks have"""
include_works: Literal[0, 1]
7 changes: 7 additions & 0 deletions tests/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,10 @@ def test_client_get_bm_cs_projects_results():
json={"data": {}},
)
client.get_bm_cs_projects_results("project-id", "stream-id")


def test_client_get_external_metadata_tracks():
client = Client(_Auth("bearer-token"))
with requests_mock.Mocker() as mock:
mock.get(f"{client.base_url}/api/external-metadata/tracks", json={"data": [{}]})
client.get_external_metadata_tracks()