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

CB-231: Retrieve artist data directly from the MB database #140

Merged
merged 5 commits into from Sep 3, 2017

Conversation

@ferbncode
Copy link
Collaborator

ferbncode commented Aug 1, 2017

Added functions for fetching artists information from mb database.
Tasks remaining:

  • Tests
  • Fetching release_groups for an artist and sorting them by release year
@ferbncode ferbncode force-pushed the ferbncode:artists branch from ab4641b to 1c2f5c1 Aug 1, 2017
@gentlecat

This comment has been minimized.

Copy link
Contributor

gentlecat commented Aug 2, 2017

If you mark pull request as work in progress can you please specify what needs to be done to make this mergeable?

@gentlecat gentlecat self-requested a review Aug 2, 2017
@ferbncode ferbncode force-pushed the ferbncode:artists branch from 1c2f5c1 to 1b33d7a Aug 13, 2017
@ferbncode ferbncode changed the title [WIP]: CB-231: Use database access for entity: artist CB-231: Use database access for entity: artist Aug 13, 2017
if includes is None:
includes = []
includes_data = defaultdict(dict)
# TODO(ferbncode): Check includes

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 14, 2017

Contributor

What do you mean by this and why can't it be done now?

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 15, 2017

Author Collaborator

Fixed this. Thanks for pointing this out. 😄


class ArtistTestCase(TestCase):


This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 14, 2017

Contributor

Should be just one blank line here. https://www.python.org/dev/peps/pep-0008/#blank-lines

self.release_group_query = self.mock_db.query.return_value.options.return_value.options.return_value.\
options.return_value.options.return_value.options.return_value.filter.return_value.all
self.release_group_query_without_artists = self.mock_db.query.return_value.\
options.return_value.options.return_value.filter.return_value.all

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 17, 2017

Contributor

Can you please decipher this? I have no idea what value comes out of this or why it has to be retrieved like that?

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 17, 2017

Author Collaborator

I have split this to make it clearer. The test function test_get_by_id fetches release groups with the include artists and the test function test_fetch_multiple_release_groups fetches release groups with no includes. Therefore, I've mocked two different queries, one with artists and other without artists.

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 19, 2017

Contributor

I'm not sure if it's any better. It would be useful if you could describe how you wrote this and how/when it should be modified in the future.


def test_fetch_browse_release_groups(self):
self.browse_release_groups_query = self.mock_db.query.return_value.options.return_value.join.return_value.\
join.return_value.join.return_value.join.return_value.filter.return_value.filter.return_value

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 17, 2017

Contributor

This is one case where it's worth splitting an infinitely deep attribute selection into multiple variables and/or function calls. I have no idea what's going on here.

@@ -87,15 +94,9 @@ def _squash_duplicated_members(members):


def _get_period(member):

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 17, 2017

Contributor

Please document what the input and output of this function is and why it's necessary.

'title': 'A Thousand Suns',
'first-release-year': 2010,
}], 1)
return ([], 0)

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 17, 2017

Contributor

I don't think parentheses around return values are necessary.

@ferbncode ferbncode force-pushed the ferbncode:artists branch from e9a1f3a to be80aa9 Aug 17, 2017
@ferbncode ferbncode force-pushed the ferbncode:artists branch from be80aa9 to ff0fe1a Aug 17, 2017
@ferbncode ferbncode force-pushed the ferbncode:artists branch from ff0fe1a to ee6d720 Aug 17, 2017
@@ -24,18 +24,16 @@ def return_release_groups(*, artist_id, release_types=None, limit=None, offset=N
'title': 'A Thousand Suns',
'first-release-year': 2010,
}], 1)
return ([], 0)
return [], 0

This comment has been minimized.

Copy link
@gentlecat

gentlecat Aug 17, 2017

Contributor

Same in the other statements, no?

This comment has been minimized.

Copy link
@ferbncode

ferbncode Aug 18, 2017

Author Collaborator

Fixed it 😅

@gentlecat gentlecat changed the title CB-231: Use database access for entity: artist CB-231: Retrieve artist data directly from the MB database Aug 20, 2017
Copy link
Contributor

gentlecat left a comment

Everything apart from very long attribute selection in tests looks good to me. This needs to be improved somehow. One option is to describe how and why you wrote these parts and how to work with them in the future.

@ferbncode

This comment has been minimized.

Copy link
Collaborator Author

ferbncode commented Aug 22, 2017

The attribute selection is a result of mocking the SQLAlchemy query which involves long attribute selection.(requring to mock return_value for each attribute.). For the browse_release_groups function, I've added a function _browse_release_groups_query that in turn returns the query. This way I've tried to avoid the long attribute selection in release_group_test.py.

@gentlecat gentlecat merged commit fbfbbd3 into metabrainz:master Sep 3, 2017
2 checks passed
2 checks passed
Jenkins [PyLint & Flake8] Build finished.
Details
Jenkins [PyTest] Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.