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

ferbncode · 2017-08-01T09:21:31Z

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

Tests
Fetching release_groups for an artist and sorting them by release year

gentlecat · 2017-08-02T16:33:12Z

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

gentlecat · 2017-08-14T20:03:10Z

critiquebrainz/frontend/external/musicbrainz_db/artist.py

+    if includes is None:
+        includes = []
+    includes_data = defaultdict(dict)
+    # TODO(ferbncode): Check includes


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

Fixed this. Thanks for pointing this out. 😄

gentlecat · 2017-08-14T20:36:03Z

critiquebrainz/frontend/external/musicbrainz_db/tests/artist_test.py

+
+class ArtistTestCase(TestCase):
+
+


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

gentlecat · 2017-08-17T14:01:08Z

critiquebrainz/frontend/external/musicbrainz_db/tests/release_group_test.py

+        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


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

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.

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.

gentlecat · 2017-08-17T14:04:05Z

critiquebrainz/frontend/external/musicbrainz_db/tests/release_group_test.py

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

gentlecat · 2017-08-17T14:06:15Z

critiquebrainz/frontend/views/artist.py

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


 def _get_period(member):


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

gentlecat · 2017-08-17T14:07:57Z

critiquebrainz/frontend/views/test/test_artist.py

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


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

gentlecat · 2017-08-17T22:17:02Z

critiquebrainz/frontend/views/test/test_artist.py

@@ -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


Same in the other statements, no?

Fixed it 😅

gentlecat

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 · 2017-08-22T16:09:51Z

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.

ferbncode force-pushed the artists branch from ab4641b to 1c2f5c1 Compare August 1, 2017 09:25

gentlecat self-requested a review August 2, 2017 21:35

ferbncode force-pushed the artists branch from 1c2f5c1 to 1b33d7a Compare August 13, 2017 09:05

ferbncode changed the title ~~[WIP]: CB-231: Use database access for entity: artist~~ CB-231: Use database access for entity: artist Aug 13, 2017

gentlecat reviewed Aug 14, 2017

View reviewed changes

gentlecat approved these changes Aug 15, 2017

View reviewed changes

gentlecat suggested changes Aug 17, 2017

View reviewed changes

ferbncode force-pushed the artists branch from e9a1f3a to be80aa9 Compare August 17, 2017 20:00

ferbncode added 2 commits August 18, 2017 01:36

CB-231: Use database access for entity: artist

f89e659

Fix indentation and check includes

34eccb2

ferbncode force-pushed the artists branch from be80aa9 to ff0fe1a Compare August 17, 2017 21:04

Split mocked test query, add docstring, syntax fixes

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

ee6d720

ferbncode force-pushed the artists branch from ff0fe1a to ee6d720 Compare August 17, 2017 21:25

gentlecat reviewed Aug 17, 2017

View reviewed changes

Fix brackets in tuple

3d3e5bb

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

gentlecat suggested changes Aug 20, 2017

View reviewed changes

Add wrapper for returning query

462c406

ferbncode mentioned this pull request Sep 2, 2017

CB-84: Add checkbox to filter releases with reviews on artist discography pages. #153

Closed

gentlecat merged commit fbfbbd3 into metabrainz:master Sep 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

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

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

ferbncode commented Aug 1, 2017 •

edited

Loading

gentlecat commented Aug 2, 2017

gentlecat Aug 14, 2017

ferbncode Aug 15, 2017

gentlecat Aug 14, 2017

gentlecat Aug 17, 2017

ferbncode Aug 17, 2017

gentlecat Aug 19, 2017

gentlecat Aug 17, 2017

gentlecat Aug 17, 2017

gentlecat Aug 17, 2017

gentlecat Aug 17, 2017

ferbncode Aug 18, 2017

gentlecat left a comment

ferbncode commented Aug 22, 2017 •

edited

Loading

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


		def _get_period(member):


		class ArtistTestCase(TestCase):

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

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

Conversation

ferbncode commented Aug 1, 2017 • edited Loading

gentlecat commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gentlecat left a comment

Choose a reason for hiding this comment

ferbncode commented Aug 22, 2017 • edited Loading

ferbncode commented Aug 1, 2017 •

edited

Loading

ferbncode commented Aug 22, 2017 •

edited

Loading