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

Implemented retrieval and/or testing of additional entities #29

Merged
merged 4 commits into from Jun 12, 2019

Conversation

@spellew
Copy link
Contributor

commented May 30, 2019

No description provided.

@ferbncode

This comment has been minimized.

Copy link

commented Jun 3, 2019

@spellew This library is used by projects both in python3 (CritiqueBrainz) and python2 (AcousticBrainz). Hence, it is required that the code is written so that the library can be used for all such projects. There are some tests failing (for python2). (You can run tests using test.sh). Please take a look :)

def _get_event_by_id(mbid):
return fetch_multiple_events(
[mbid],
includes=['artist-rels', 'place-rels', 'series-rels', 'url-rels', 'release-group-rels'],

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 3, 2019

The includes shouldn't be passed. The user of this function (CB) should be responsible for passing in the includes. Please see how this methods are implemented for already present entities (For instance, https://github.com/metabrainz/brainzutils-python/blob/master/brainzutils/musicbrainz_db/recording.py#L11).

).get(mbid)


def fetch_multiple_events(mbids, *, includes=None):

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 3, 2019

Keyword-only arguments is a feature for python3. It would be good to use keyword arguments since we would be using this module in Python2 projects as well.

def _get_place_by_id(mbid):
return fetch_multiple_places(
[mbid],
includes=['artist-rels', 'place-rels', 'release-group-rels', 'url-rels'],

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 3, 2019

Same for other entities (specifying the includes should be done by the user and the library should handle all kinds of includes).

@spellew spellew force-pushed the spellew:additional-entity-support branch 4 times, most recently from 7f49899 to aebe4f2 Jun 4, 2019

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Updated pull request per your suggestions.

@ferbncode
Copy link

left a comment

There are a few more changes and a few nitpicks. But overall, looks like a nice PR. Good work :) 🎖

from mock import MagicMock
from brainzutils.musicbrainz_db import place as mb_place
from brainzutils.musicbrainz_db.test_data import place_suisto, place_verkatehdas
import brainzutils.musicbrainz_db.utils as mb_utils

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 6, 2019

Unused import, I think we can remove this.

)


def _get_event_by_id(mbid, includes=None):

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 6, 2019

I guess we don't need these separate functions like _get_event_by_id. These functions are very similar to functions which call them, for example get_event_by_id. In CB code, there were two separate functions as caching was involved.

return release_groups


def browse_release_groups(artist_id, release_types=None, limit=None, offset=None):

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 6, 2019

Lets change the function name to get_release_groups_for_artist(). What do you think? The name of the function doesn't make it obvious on what it is supposed to do. Now that this function is part of a library, I think it makes more sense that the functions are easily understandable.

from mock import MagicMock
from brainzutils.musicbrainz_db import event as mb_event
from brainzutils.musicbrainz_db.test_data import taubertal_festival_2004, event_ra_hall_uk
import brainzutils.musicbrainz_db.utils as mb_utils

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 6, 2019

Unused import

@spellew spellew force-pushed the spellew:additional-entity-support branch 3 times, most recently from b8bc9e2 to e80a5af Jun 6, 2019


def get_many_recordings_by_mbid(mbids, includes=None):

This comment has been minimized.

Copy link
@ferbncode

ferbncode Jun 9, 2019

@spellew Let's not remove public functions (without _) in already existing code. It might be a breaking change for code if it's already used somewhere.

@ferbncode
Copy link

left a comment

The pull request looks good. Just one more comment that I've added for changes since the last commit and we should be good to go.

@spellew spellew force-pushed the spellew:additional-entity-support branch 2 times, most recently from 08846b8 to b889db3 Jun 9, 2019

@spellew spellew closed this Jun 10, 2019

@spellew spellew force-pushed the spellew:additional-entity-support branch from b889db3 to 3738c96 Jun 10, 2019

@paramsingh paramsingh reopened this Jun 10, 2019

@ferbncode

This comment has been minimized.

Copy link

commented Jun 10, 2019

@spellew Also, make sure to not use the -f option while pushing changes to pull request. Force pushes make it hard to review changes and keep track of what sub-changes might be missing :)

@ferbncode
Copy link

left a comment

This looks good 🏅 We can come back to some minor changes in a separate pull request if necessary.

@spellew

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Ok, feel free to close it and merge.

@paramsingh paramsingh merged commit a82fc5b into metabrainz:master Jun 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.