Added Lyrics w. Timestamps #662

heinrich26 · 2024-10-10T18:38:00Z

Added get_lyrics_with_timestamps to get lyrics with timestamps. The Method doesn't try to parse the response as normal lyrics, if no lyrics with timestamps are returned. (could be changed to do so tho, the format is the same)

Added `get_lyrics_with_timestamps` to get lyrics with timestamps. The Method doesn't try to parse the response as normal lyrics, if no lyrics with timestamps are returned. (could be changed to do so tho, the format is the same)

codecov · 2024-10-11T15:53:11Z

Codecov Report

Attention: Patch coverage is 97.61905% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.99%. Comparing base (2b5d19a) to head (a8a1f5f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ytmusicapi/mixins/browsing.py	92.85%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   94.99%   94.99%   -0.01%     
==========================================
  Files          38       40       +2     
  Lines        2278     2337      +59     
==========================================
+ Hits         2164     2220      +56     
- Misses        114      117       +3

Flag	Coverage Δ
unittests	`94.99% <97.61%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sigma67

Hi @heinrich26 , pretty cool that you were able to make this work!

I take it you're basically emulating the mobile client to fetch the timed data? Since you're modifying the client info.

Could we replace the previous get_lyrics with this? I wouldn't want to have two methods for the same purpose in the API if one provides a superset of information.

Finally, to get this merged:

please ensure the linters pass
please add a test for the function

heinrich26 · 2024-10-11T23:44:19Z

Yea, calling the API "as a different frontend" gives you different results. I temporarily changed the client because I thought unforeseen side-effects aren't desirable.As per the implementation of a multipurpose `get_lyrics()`, I would add an argument like `timestamps=True/False`, to keep it backwards compatible and allow the user to always get the same (old) format – with my approach, you'll end up with 2 possible formats.Furthermore, would you keep the original format with the nested `CueRange` or would you rather have a flat dict? I think that nesting is kinda useless.

… errors

…ads, because vscode didn't show them otherwise

Pushed to the wrong branch :(

because idk where it came from...

sigma67

Thanks, looks much better! :)

As a general rule of thumb: Don't change more than necessary in a single PR. It makes it harder for the maintainer to review the changes (more time consuming), and harder to trace back potential future issues to specific PRs. It will also prolong the time it takes for the PR to get merged.

Note for this review: Please don't resolve threads, I will do so myself after verifying the changes

sigma67 · 2024-10-15T14:49:44Z

ytmusicapi/mixins/search.py

@@ -204,7 +206,7 @@ def search(
        if filter and "playlists" in filter:
            filter = "playlists"
        elif scope == scopes[1]:
-            filter = scopes[1]
+            filter = scopes[1] # type:ignore


please undo all changes to this file

ytmusicapi/mixins/uploads.py

sigma67 · 2024-10-15T14:50:51Z

ytmusicapi/ytmusic.py

@@ -181,7 +181,7 @@ def __init__(
            try:
                cookie = self.base_headers.get("cookie")
                self.sapisid = sapisid_from_cookie(cookie)
-                self.origin = self.base_headers.get("origin", self.base_headers.get("x-origin"))
+                self.origin = cast(str, self.base_headers.get("origin", self.base_headers.get("x-origin")))


Please undo changes to this file

sigma67 · 2024-10-15T14:52:15Z

ytmusicapi/__init__.py

@@ -2,14 +2,16 @@

 from ytmusicapi.setup import setup, setup_oauth
 from ytmusicapi.ytmusic import YTMusic
+from .mixins.browsing import Lyrics, TimedLyrics, LyricLine


Please undo the changes to this file. They are not needed and I don't want to start exporting everything from root

I was thinking similar, but then you'll need to create a submodule for these classes to be accessible

You can already access them: from ytmusicapi.mixins.browsing import TimedLyrics

It's similar for https://github.com/sigma67/ytmusicapi/blob/main/ytmusicapi/parsers/podcasts.py

Although with #621 in mind, it might be wise to move them to a ytmusicapi.models module (one for podcasts and one for lyrics, I guess)

sigma67 · 2024-10-15T14:53:31Z

ytmusicapi/mixins/_utils.py


 from ytmusicapi.exceptions import YTMusicUserError


+OrderType = Literal['a_to_z', 'z_to_a', 'recently_added']


This is a good change in general, but the name is too broad. Since you introduced an ArtistOrderType elsewhere, this should have a more specific name as well. Maybe LibraryOrderType ?

sigma67 · 2024-10-15T15:02:27Z

ytmusicapi/mixins/browsing.py

-        return lyrics
+        if timestamps:
+            # restore the old context
+            self.context["context"]["client"] = copied_context_client  # type: ignore


type ignore should not be needed

this is technically not a type ignore, but it will supress the typechecker error:

copied_context_client might not be initialized

sigma67 · 2024-10-15T15:06:58Z

ytmusicapi/mixins/browsing.py

+
+        if timestamps:
+            # change the client to get lyrics with timestamps (mobile only)
+            copied_context_client = self.context["context"]["client"].copy()


This could be used in more places in the future, so I would suggest moving it to a separate utility function that returns a deepcopy of the context changed to the mobile context (maybe get_mobile_context next to initialize_context in helpers.py)

Agreed, however I wouldn't deepcopy the context, because someone might want to do weird stuff with references inside self.context. Using shallow-copy here, should structually restore everything to how it was before the call. (so I think)

a cool thing would be using a context manager for use with, like

with yt.as_mobile(): ... # back to desktop

(contextlib) makes a function a context-manager

Interesting idea, I like it. But it won't be pretty to wrap the entire function body in a with statement?

And wouldn't it cause problems during parallel execution? If I call a two endpoint with the same YTMusic instance at the same time, wouldn't one of them break if you perform a shallow copy?

ytmusicapi/mixins/browsing.py

sigma67 · 2024-10-15T15:11:03Z

ytmusicapi/mixins/browsing.py

+    @overload
+    def get_lyrics(self, browseId: str, timestamps: Literal[True] = True) -> Optional[Lyrics|TimedLyrics]:
+        """
+        Returns lyrics of a song or video. When `timestamps` is set, lyrics are returned with


The docstring is now duplicated in three places, please fix

Only the actual function should have the docstring, overloads don't need it

sigma67 · 2024-10-15T15:11:32Z

ytmusicapi/mixins/browsing.py

@@ -876,7 +1068,7 @@ def get_basejs_url(self):
        if match is None:
            raise YTMusicError("Could not identify the URL for base.js player.")

-        return YTM_DOMAIN + match.group(1)
+        return cast(str, YTM_DOMAIN + match.group(1))


please don't change unrelated functions

cast doesn't do anything but assert a type to the type checker.
However, this should be removed and instead a return type should be added. (fixes the same issue)

ok, could you either revert the change or add the return type then? I'd prefer the latter

sigma67 · 2024-10-15T15:15:21Z

ytmusicapi/mixins/_protocol.py


    def _send_request(self, endpoint: str, body: dict, additionalParams: str = "") -> dict:
        """for sending post requests to YouTube Music"""
+        ...


The dots are not needed, please remove?

Pylance will report an error if not.
(if you would remove the docstring, it would throw an error anyway, as the function is technically empty. See: https://docs.python.org/3/library/typing.html#typing.overload)

This project uses mypy, so please don't worry about pylance findings. For mypy it's ok to have a docstring in place of ...

It's a matter of personal preference - for me the docstring has a purpose and fulfills the requirement of no empty function body, so I like it better than the nondescript ...

Side note: I have triggered the lint jobs so you can see the mypy and ruff findings remaining.

I recommend taking a look at https://github.com/sigma67/ytmusicapi/blob/main/CONTRIBUTING.rst

where pre-commit is described - if that passes, the lint jobs should pass as well.

reverted some changes that I moved to an extra PR

…y `yt.as_mobile()`

heinrich26 · 2024-10-20T11:50:00Z

finally used pre-commit to fix those formatting complaints 🤷‍♂️

heinrich26 added 2 commits October 10, 2024 20:37

Added Lyrics w. Timestamps

4d4a76f

Added `get_lyrics_with_timestamps` to get lyrics with timestamps. The Method doesn't try to parse the response as normal lyrics, if no lyrics with timestamps are returned. (could be changed to do so tho, the format is the same)

Update browsing.py

14313be

sigma67 reviewed Oct 11, 2024

View reviewed changes

heinrich26 and others added 4 commits October 12, 2024 20:14

Combined both get_lyrics methods into one and fixed some typechecking…

0885616

… errors

Fixed a missing hasTimestamps and added the doccomments to the overlo…

28a8828

…ads, because vscode didn't show them otherwise

Merge pull request #1 from heinrich26/main

ab032f5

Pushed to the wrong branch :(

Removed the old get_lyrics_with_timestamps method

049b0b0

because idk where it came from...

sigma67 reviewed Oct 15, 2024

View reviewed changes

sigma67 added the enhancement New feature or request label Oct 16, 2024

heinrich26 and others added 3 commits October 16, 2024 23:33

fixed remaining issues

3198b1f

Update uploads.py

cfd9a4a

reverted some changes that I moved to an extra PR

removed variable context from the Mixin, as it's use was replaced b…

0c2444c

…y `yt.as_mobile()`

heinrich26 requested a review from sigma67 October 16, 2024 21:58

heinrich26 added 2 commits October 18, 2024 18:26

fix some formatting complaints by ruff and mypy

a8a1f5f

please the linter

b546eed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Lyrics w. Timestamps #662

Added Lyrics w. Timestamps #662

heinrich26 commented Oct 10, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

sigma67 left a comment

heinrich26 commented Oct 11, 2024 via email

sigma67 left a comment

sigma67 Oct 15, 2024

sigma67 Oct 15, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 16, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 16, 2024

sigma67 Oct 15, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 16, 2024

sigma67 Oct 15, 2024

heinrich26 Oct 15, 2024

sigma67 Oct 16, 2024

sigma67 Oct 16, 2024

heinrich26 commented Oct 20, 2024


		from ytmusicapi.exceptions import YTMusicUserError


		OrderType = Literal['a_to_z', 'z_to_a', 'recently_added']

Added Lyrics w. Timestamps #662

Are you sure you want to change the base?

Added Lyrics w. Timestamps #662

Conversation

heinrich26 commented Oct 10, 2024

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

sigma67 left a comment

Choose a reason for hiding this comment

heinrich26 commented Oct 11, 2024 via email

sigma67 left a 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

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

heinrich26 commented Oct 20, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading