CB-231: Retrieve artist data directly from the MB database #140
Conversation
|
If you mark pull request as work in progress can you please specify what needs to be done to make this mergeable? |
ferbncode
changed the title
| if includes is None: | ||
| includes = [] | ||
| includes_data = defaultdict(dict) | ||
| # TODO(ferbncode): Check includes |
There was a problem hiding this comment.
What do you mean by this and why can't it be done now?
There was a problem hiding this comment.
Fixed this. Thanks for pointing this out. 😄
|
|
||
| class ArtistTestCase(TestCase): | ||
|
|
||
|
|
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Can you please decipher this? I have no idea what value comes out of this or why it has to be retrieved like that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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): | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
I don't think parentheses around return values are necessary.
| @@ -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 | |||
There was a problem hiding this comment.
Same in the other statements, no?
gentlecat
changed the title
|
The attribute selection is a result of mocking the SQLAlchemy query which involves long attribute selection.(requring to mock |
Added functions for fetching artists information from mb database.
Tasks remaining:
release_groupsfor an artist and sorting them by release year