Add parser for reading from the song's tags or from a local .lrc/.lyric file by alpemwarrior · Pull Request #79 · timoloewe/lLyrics

alpemwarrior · 2020-07-29T10:25:41Z

The user can select "Local File" as a source, which will fetch lyrics from the song's metadata (mp3, ogg, opus and flac only) or from a .lrc or .lyric file with the same filename in the same path.
Also edited the README to mention the use of the mutagen library.

or from the metadata of the file. Supports mp3, ogg, opus and flac formats

…of the mutagen module.

ratijas

Nice going! Just a few comments from me.

PS GitHub's ordering of comments is messed up, comparing to their top-to-bottom order, but you should get the idea.

ratijas · 2020-07-29T12:26:37Z

+        try:
+            out = file["LYRICS"]
+        except:
+            pass
+        try:
+            out = file["UNSYNCEDLYRICS"]
+        except:
+            pass


same.

You know, at this point, the whole file[...] retrieval deserves it's own function.

ratijas · 2020-07-29T12:26:49Z

+        try:
+            out = file["LYRICS"]
+        except:
+            pass
+        try:
+            out = file["UNSYNCEDLYRICS"]
+        except:
+            pass


ratijas · 2020-07-29T12:27:13Z

+        try:
+            out = file["LYRICS"]
+        except:
+            pass
+        try:
+            out = file["UNSYNCEDLYRICS"]
+        except:
+            pass


This is weird. In case both indexing operations succeed, second would just overwrite the variable. If they are mutually exclusive, could you explicitly mark them as such, please?

Nope, you are right

ratijas · 2020-07-29T12:28:45Z

@@ -0,0 +1,123 @@
+# -*- coding: utf-8 -*-


I wonder, is it still relevant in 2020, after Python-2's demise?

Yep, it's useless. I just copied the template from another parser.

ratijas · 2020-07-29T12:32:05Z


 LYRICS_SOURCES = ["Lyricwiki.org", "Letras.terra.com.br", "Metrolyrics.com", "AZLyrics.com", "Lyricsmania.com",
-                  "Vagalume.com.br", "Genius.com", "Darklyrics.com", "Chartlyrics.com"]
+                  "Vagalume.com.br", "Genius.com", "Darklyrics.com", "Chartlyrics.com", "Local File"]


Is there any i18n opportunity for plugins?

ratijas · 2020-07-29T12:33:05Z

+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import urllib
+import os.path as path


FWIW there's a modern pathlib out there, but otherwise it's not really much different.

ratijas · 2020-07-29T12:40:50Z

+        elif path.isfile(dir + self.title + ".lyric"):
+            return open(dir + self.title + ".lyric")
+        else:
+            return ""


So, this function returns either opened file object or a string. Sounds a little bit off. Better return None as usual, and check for it via is operator. Return type will be Optional[file] (well, kind of — since the open's real type would be _io._IOBase or its derivative).

alpemwarrior · 2020-07-29T13:01:01Z

Thanks for the feedback! I'll rewrite it and update it. No idea about how the translation stuff works, but I'll look into it.
Also, there is an ugly patch in lLyrics.py because the parser needs the song url to work. Is this fine or is there a more elegant way to do it?

alpemwarrior · 2020-07-29T13:49:36Z

I have looked into the translation stuff and it should be easy to do. Can't update any of them tho.
I decided not to use pathlib because the object syntax is just weird for what i'm trying to do, and os.path is not deprecated or anything. Other than that, everything else has been fixed.

ratijas

That's definitely going somewhere the right way! Just few more nits, if you please.

ratijas · 2020-07-29T16:37:24Z

@@ -0,0 +1,105 @@
+# Parses lyrics from a .lyric or .lrc file or from the tags of the song


I don't get it why this repository has no global LICENSE file, or why GPL text is fragmented via blank lines like that, but one thing is for sure: doc comments should be separate, inside triple double quotes and come after license text.

Also, lets call it "track" instead of "song". Even audio books and podcasts have 'lyrics' in them sometimes.

Suggested change

# Parses lyrics from a .lyric or .lrc file or from the tags of the song

"""

Parses lyrics from a .lyric or .lrc file or from the tags of the track.

"""

ratijas · 2020-07-29T16:38:56Z

+    def __init__(self, artist, title, song_path):
+        self.artist = artist
+        self.title = title
+        self.lyrics = ""


Unused member self.lyrics.

ratijas · 2020-07-29T16:44:16Z

+        # This only checks for .lrc or .lyric files with the same name as the song or with the same "cleaned" name as
+        # the song. If you have files in any other format, please add it to this function.


Doc comments

Suggested change

# This only checks for .lrc or .lyric files with the same name as the song or with the same "cleaned" name as

# the song. If you have files in any other format, please add it to this function.

"""

This only checks for .lrc or .lyric files with the same name as the song or with the same "cleaned" name as

the song. If you have files in any other format, please add it to this function.

"""

ratijas · 2020-07-29T17:29:36Z

+        elif path.endswith("flac"):
+            out = self.get_flac_lyrics(path)
+        if out is None:
+            file = self.check_for_file(dir, file_name)


Name suggests it only checks, but in fact it opens a path and returns opened file. Since open calls are not guarded anyway, wouldn't it make more sense to return a string path here, and call open only once?

yeah returning open() is kinda sketchy anyway.

ratijas · 2020-07-29T17:34:59Z

+            out = ""
+            for line in file.readlines():
+                out += line
+            file.close()


No need to read line-by-line, and no need to manually close it with this syntax:

Suggested change

file.close()

with open(path) as file:

out = file.read()

neat! Didn't know you could do that.

alpemwarrior · 2020-07-29T20:18:08Z

Thanks again for your time and feedback!

ratijas · 2020-08-03T18:33:12Z

Hi, @dmo60. I think this is ready to squash & merge. Do you think you can take a look?

ratijas · 2020-08-19T16:02:00Z

@dmo60 Sorry to bump you on this, but it is kind of stagnating. Is there anything blocking from merge & release?

Alternatively, you could add me as a maintainer. I am skillful with Python and many other stuff, and I got a load of free time lately.

wolf-yuan-6115 · 2023-12-19T14:29:31Z

@dmo60 Hello, any news for merging this into master?

rexendevar · 2025-02-25T09:29:28Z

dang i sure wish i had read the merge requests before duplicating all that effort lmao. and at the end of it all mine is worse and coded more poorly

Pol Rius added 3 commits July 28, 2020 18:03

Adds a new parser that gets the lyrics from a local .lrc or .lyric file

bb948ad

or from the metadata of the file. Supports mp3, ogg, opus and flac formats

Some bugfixes on LocalParser.py

87a03e6

Removed unnecessary library and edited the README to mention the use …

6cc0899

…of the mutagen module.

alpemwarrior mentioned this pull request Jul 29, 2020

Show lyrics from tags #67

Open

ratijas suggested changes Jul 29, 2020

View reviewed changes

Cleaning and rewriting stuff

d0a9be9

ratijas suggested changes Jul 29, 2020

View reviewed changes

Cleaning and changing stuff

dc454a5

		@@ -0,0 +1,105 @@
		# Parses lyrics from a .lyric or .lrc file or from the tags of the song

		# This only checks for .lrc or .lyric files with the same name as the song or with the same "cleaned" name as
		# the song. If you have files in any other format, please add it to this function.

Conversation

alpemwarrior commented Jul 29, 2020

Uh oh!

ratijas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ratijas Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpemwarrior commented Jul 29, 2020

Uh oh!

alpemwarrior commented Jul 29, 2020

Uh oh!

ratijas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpemwarrior commented Jul 29, 2020

Uh oh!

ratijas commented Aug 3, 2020

Uh oh!

ratijas commented Aug 19, 2020

Uh oh!

wolf-yuan-6115 commented Dec 19, 2023

Uh oh!

rexendevar commented Feb 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ratijas left a comment •

edited

Loading

ratijas Jul 29, 2020 •

edited

Loading