Added total_tracks to SoundCloudAlbum #12
Conversation
|
Thanks for the PR! We need to add this property to the following mixin as well: This is the mixin used by SoundcloudAlbum to get properties and is also what is used to generate the docs found here: (Sorry, I haven't documented this very well yet) Please use Numpy docstring format. It's used in the mixin. Lastly, we need to add this property into our unit tests. See https://github.com/jmbannon/ytdl-sub/blob/e8e1daaec29f39f5113f772004595a159fe4ce55/tests/unit/entries/test_soundcloud_entries.py You can run the tests with That was a lot! I plan to make this more clear somewhere for contributors to see. If you need any help I'd be happy to hop on discord or something sometime |
|
Still, working on it hope you don't mind just taking the time to understand the architecture. |
…to entries/variables/soundcloud_variables
|
Ok, I've added it to the SoundCloudVariables mixin, added it to the doc, added it to the unit tests and ran the linter and tests on it and it passed. |
| ------- | ||
| The total tracks in album. For singles, it will always be 1. | ||
| """ | ||
| if self.kwargs_contains("total_tracks"): |
There was a problem hiding this comment.
the kwargs here is referring to the YTDL dict returned for the soundcloud entry. I don't think total_tracks is there. We can always return 1 here just like we do with track_number
There was a problem hiding this comment.
Then SoundcloudAlbumTrack will override and use its value
There was a problem hiding this comment.
Yes, didn't release what kwargs was actually doing, changed it, to that implementation.
src/ytdl_sub/entries/soundcloud.py
Outdated
| @@ -105,6 +112,7 @@ def album_tracks(self, skip_premiere_tracks: bool = True) -> List[SoundcloudAlbu | |||
| album=self.title, | |||
| album_year=self.album_year, | |||
| soundcloud_track=track, | |||
| total_tracks=self.track_count, | |||
There was a problem hiding this comment.
maybe we should incorporate this into the PlaylistMetadata as playlist_count. Then in total_tracks, return self._playlist_metadata.playlist_count
I think the ytdl field is 'playlist_count', i recommend using a debugger and seeing what ytdl returns in the entry dict
There was a problem hiding this comment.
That would help solve this issue: #9
There was a problem hiding this comment.
Yea that makes sense, I've incorporated it.
There was a problem hiding this comment.
I've also launched it in a debugger and checked the entry dict, and the field is playlist_count, this is also used in the track_count property in SoundcloudAlbum.
There was a problem hiding this comment.
Sweet looks great! Looks like the linter errored: https://github.com/jmbannon/ytdl-sub/runs/6236060213?check_suite_focus=true
One last nit: lets change the soundcloud variable name from total_tracks to track_count, that way it's near the other track variables via sorting in the documentation
There was a problem hiding this comment.
I believe I've fixed that linter error, and I've changed the variable name.
…, and added tmp and vs folders to .gitignore
src/ytdl_sub/entries/soundcloud.py
Outdated
| @@ -51,6 +51,11 @@ def album(self) -> str: | |||
| """Returns the entry's album name, fetched from its internal album""" | |||
| return self._album | |||
|
|
|||
| @property | |||
| def total_tracks(self) -> int: | |||
There was a problem hiding this comment.
need to set this to track_count
| @pytest.fixture() | ||
| def track_count(): | ||
| return 1 |
There was a problem hiding this comment.
after these two small fixes it should be good to merge
There was a problem hiding this comment.
Fixed those two problems, thanks for being patient this has been quite the pull request at least it has been enjoyable.
There was a problem hiding this comment.
No problem, thank you for contributing! I hope my comments have been helpful and that you find this app useful
The property will get the total tracks of the sound cloud album via the album_tracks method. If the sound cloud album is a single it will return 1.