Add parser for reading from the song's tags or from a local .lrc/.lyric file #79

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

lLyrics/LocalParser.py

+        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

lLyrics/LocalParser.py

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


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

lLyrics/LocalParser.py

+        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

lLyrics/LocalParser.py

@@ -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

lLyrics/lLyrics.py

@@ -94,7 +95,7 @@
 LYRICS_ARTIST_REPLACE = [("/", "-"), (" & ", " and ")]

 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

lLyrics/LocalParser.py

+# 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

lLyrics/LocalParser.py

+        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

lLyrics/LocalParser.py

@@ -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

lLyrics/LocalParser.py

+    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

lLyrics/LocalParser.py

+        # 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

lLyrics/LocalParser.py

+        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

lLyrics/LocalParser.py

+            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?

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parser for reading from the song's tags or from a local .lrc/.lyric file #79

Add parser for reading from the song's tags or from a local .lrc/.lyric file #79

alpemwarrior commented Jul 29, 2020

ratijas left a comment •

edited

Loading

ratijas Jul 29, 2020

ratijas Jul 29, 2020

ratijas Jul 29, 2020

alpemwarrior Jul 29, 2020

ratijas Jul 29, 2020

alpemwarrior Jul 29, 2020

ratijas Jul 29, 2020

ratijas Jul 29, 2020

ratijas Jul 29, 2020 •

edited

Loading

alpemwarrior commented Jul 29, 2020

alpemwarrior commented Jul 29, 2020

ratijas left a comment

ratijas Jul 29, 2020

ratijas Jul 29, 2020

ratijas Jul 29, 2020

ratijas Jul 29, 2020

alpemwarrior Jul 29, 2020

ratijas Jul 29, 2020

alpemwarrior Jul 29, 2020

alpemwarrior commented Jul 29, 2020

ratijas commented Aug 3, 2020

ratijas commented Aug 19, 2020

wolf-yuan-6115 commented Dec 19, 2023

		@@ -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.

Add parser for reading from the song's tags or from a local .lrc/.lyric file #79

Are you sure you want to change the base?

Add parser for reading from the song's tags or from a local .lrc/.lyric file #79

Conversation

alpemwarrior commented Jul 29, 2020

ratijas left a comment • edited Loading

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

ratijas Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

alpemwarrior commented Jul 29, 2020

alpemwarrior commented Jul 29, 2020

ratijas 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

alpemwarrior commented Jul 29, 2020

ratijas commented Aug 3, 2020

ratijas commented Aug 19, 2020

wolf-yuan-6115 commented Dec 19, 2023

ratijas left a comment •

edited

Loading

ratijas Jul 29, 2020 •

edited

Loading