get_album: parse more data from response + correct keys on other_vers… #523
Conversation
| @@ -54,13 +54,23 @@ def parse_content_list(results, parse_func, key=MTRIR): | |||
| def parse_album(result): | |||
| return { | |||
| "title": nav(result, TITLE_TEXT), | |||
| "year": nav(result, SUBTITLE2, True), | |||
There was a problem hiding this comment.
YT may have moved something, on all the albums I've tried that nav route gives you the album's artist. I can't find year anywhere.
There was a problem hiding this comment.
This function has multiple usages (via parse_mixed_content (get_home, get_song_related), parse_artist_contents (get_artist, get_user)), please double-check that this is in fact returning None for all of them
There was a problem hiding this comment.
Looks like get_home's return is the same as get_album()["other_versions"], so it's also incorrectly returning artist names as the year.
get_song_related and get_artist do correctly return year with the old version of parse_album, but could still benefit from type and audioplaylistid returns of the PR version.
get_user appears to be incapable of actually returning albums despite having access to the function, so I'm not sure on that one unless you have an ID I could test against.
I'll rework the PR to flip between artists and year return based on the context.
| "thumbnails": nav(result, THUMBNAIL_RENDERER), | ||
| "isExplicit": nav(result, SUBTITLE_BADGE_LABEL, True) is not None, | ||
| } | ||
|
|
||
|
|
||
| def parse_id_name(sub_run): |
There was a problem hiding this comment.
this function name is a bit generic, maybe choose something more specific like parse_run_browseid? parse_run_artist?
There was a problem hiding this comment.
I intentionally left it generic as the same pattern appears outside of artists. Didn't commit any related changes in this PR, but just about anything with a title and browse id could be pulled with it. That being said, its more than likely a weird duplicate of some other function I didn't see.
There was a problem hiding this comment.
There's not yet another function like it, so that is fine. Functions with similar parsing: parse_song_flat, parse_song_artist_runs, parse_song_runs
parse_song_runs and parse_song_artist_runs are places where you could use this function as well. I suggest refactoring these functions to use it
parse_song_flat is an example where you'd parse an album name and id - which works differently. Therefore I'd at least include the word artist in the function name: parse_artist_id_name - as artist is the only context we know where it works
There was a problem hiding this comment.
Hi @jcbirdwell , please don't take this the wrong way, but your last commit went too far beyond the scope of this PR and the code/intention isn't really clear to me (especially what you did in parse_pl_song_artists and parse_playlist - parse_video also seemed unfinished).
So to include the original fixes in the next release I'm reverting the most recent commit and finishing up. Feel free to resubmit changes from the last commit in a separate PR.
sigma67
force-pushed
the
album-data-mining
branch
from
6215643
to
10b0546
Compare
#522 + tests. I'll bet there's still more to be found, but I'm going cross eyed trying to read these response objects. This fixes the original issue of incorrect keys in "other_versions" and adds audio playlist id, artists with id and name, album type, removes year (I couldn't find it), and adds explicit to the main album requested in get_album.