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

AB-21: Get high-level data for multiple recordings with one request #261

Merged
merged 6 commits into from Feb 18, 2019

Conversation

Projects
None yet
3 participants
@rsh7
Copy link
Contributor

rsh7 commented Feb 26, 2018

This is a...

  • Bug Fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other

Present Case:
We don't have any feature which provides high-level data with one request for many recording ids.

Solution:
I have added a feature through which we can fetch JSON documents for a list of MusicBrainz recording IDs for high-level data.
The user can construct the endpoint (Suppose for a list of 3 MBIDs) for high-level data by:
GET https://acousticbrainz.org/api/v1/high-level?recording_ids=mbid;mbid;mbid

@paramsingh
Copy link
Member

paramsingh left a comment

There is a bunch of code duplication with get_many_lowlevel. We should consider moving the common code to a function and call that function in the endpoints.

something like

def get_data_for_multiple_recordings(get_data):
    """ Gets data using the function get_data
    """

def get_many_lowlevel():
     get_data_for_multiple_recordings(db_data.load_low_level)

def get_many_highlevel():
    get_data_for_multiple_recordings(db_data.load_high_level)
@paramsingh

This comment has been minimized.

Copy link
Member

paramsingh commented Feb 27, 2018

Also, should probably add a test for the new endpoint.

@bp_core.route("/low-level", methods=["GET"])
@crossdomain()
def get_many_lowlevel():
"""Get low-level data for many recordings at once.

This comment has been minimized.

@paramsingh

paramsingh Feb 27, 2018

Member

The previous docstring that was here was used to automatically generate documentation on readthedocs for the endpoint. We should keep it and add a similar docstring for the new endpoint added.

@rsh7 rsh7 changed the title AB-21: Getting highlevel data for multiple recordings with one request AB-21: Get high-level data for multiple recordings with one request Mar 4, 2018

@rsh7 rsh7 force-pushed the rsh7:hl branch from a8c07ac to 6b33c1a Mar 31, 2018

@paramsingh paramsingh self-assigned this May 4, 2018

@@ -155,6 +155,37 @@ def _parse_bulk_params(params):
return [x for x in ret if not (x in seen or seen.add(x))]


def check_bad_request_for_multiple_recordings():
"""Checking whether the recording ids throw exceptions

This comment has been minimized.

@paramsingh

paramsingh May 4, 2018

Member

You should mention exactly what this function checks for and what exceptions are thrown if those conditions are not specified.

@rsh7 rsh7 force-pushed the rsh7:hl branch from 9c59fb6 to 3dbee9d Aug 19, 2018

@rsh7

This comment has been minimized.

Copy link
Contributor Author

rsh7 commented Aug 19, 2018

@paramsingh, does this PR need more changes?

@rsh7 rsh7 force-pushed the rsh7:hl branch from 08b9b98 to 3e00da2 Feb 15, 2019

@rsh7

This comment has been minimized.

Copy link
Contributor Author

rsh7 commented Feb 15, 2019

Rebased to master!

@alastair alastair merged commit 3e00da2 into metabrainz:master Feb 18, 2019

1 check passed

Jenkins Build finished.
Details
@alastair

This comment has been minimized.

Copy link
Contributor

alastair commented Feb 18, 2019

good patch, thanks so much. sorry for taking so long to merge it 😱

@rsh7

This comment has been minimized.

Copy link
Contributor Author

rsh7 commented Feb 21, 2019

Thank you for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.