Solved: Read media info from tags #30

DipayanP007 · 2022-01-16T10:59:38Z

Modified Code so that:

When the command is !list, the output will have audio title, artist, album in the format title | artist | album
If the media does not have the specified tags, the filename will be used instead

Screenshots:

!list command

!play command

Solves the issue #25

anoadragon453 · 2022-01-16T14:18:16Z

Thanks @DipayanP007! If you find yourself battling with the CI, note that there is a script (scripts-dev/lint.sh) that will fix/point out style errors locally. Thus, you can fix them before pushing a commit to GitHub.

The CI is just running the same commands as that script. So if the script passes, then the CI should to!

DipayanP007 · 2022-01-16T14:39:11Z

Hi @anoadragon453 thanks for the info. I did the same and the script ran on local. I guess the ci will pass now.

DipayanP007 · 2022-01-16T14:39:28Z

And review the changes if you can 😄

Cephian

Hey Dipayan, awesome work! Thanks for the PR.

I have a few formatting suggestions/requests:

The "Album" field unnecessarily clutters the output, So I think it's best to just not display that one
Instead of the format @user - <Artist Tag> | <Title Tag> I think @user: <Artist Tag> - <Title Tag> is closer to what's more commonly used for titles, so we should prefer it. Files without tags should display @user: <Filename> for consistency, where the filename is processed as it is now.
If a user submits a song with a Title tag but no Artist tag, we should just display @user: <Title Tag>

With these changes (and @anoadragon453 and I's suggestions below) I think this should be ready to commit!

Cephian · 2022-01-16T14:57:12Z

main.py

+    if len(audio.title.strip()) == 0:
+        filename = path.splitext(filename)[0]
+
+        filename = filename.replace("_", " ")


I believe we can just remove this line, since filename.replace("-", " ") is called on the line afterwards as well

Suggested change

filename = filename.replace("_", " ")

anoadragon453

Some things I noticed. Thanks again for picking this up!

anoadragon453 · 2022-01-16T14:46:28Z

.gitignore

+venv
+__pycache__


A small nitpick, but for consistency with the other entries in this file.

Suggested change

venv

__pycache__

venv/

__pycache__/

anoadragon453 · 2022-01-16T14:50:19Z

main.py

@@ -2,6 +2,7 @@
 import os


Could you remove the TODO: Parse mp3 tags and things at line ~350? This PR should resolve the TODO!

anoadragon453 · 2022-01-16T14:51:31Z

main.py

-    # Replace all underscores with spaces
-    filename = filename.replace("_", " ")
+    # Get all the tags for a track
+    audio = TinyTag.get(f"{attachment_directory_filepath}/{filename}")


Note that {attachment_directory_filepath}/{filename} will be invalid on Windows, as Windows uses \ as a directory separator.

Instead, one should use os.path.join, which will create a path suitable for the detected operating system.

anoadragon453 · 2022-01-16T14:52:02Z

main.py


-    return discord.utils.escape_markdown(filename)
+    # If the tag doesnot exist display the file name only


Suggested change

# If the tag doesnot exist display the file name only

# If the tag does not exist or only contains whitespace, display the file name only

anoadragon453 · 2022-01-16T14:58:13Z

main.py


-    return discord.utils.escape_markdown(filename)
+    # If the tag doesnot exist display the file name only
+    if len(audio.title.strip()) == 0:


Note that the attributes of audio here, such as audio.title, will be None if the corresponding tag isn't found in the file. In that case, audio.title.strip() will raise an Exception as strip() does not exist on type None.

We should first check if audio.title is None before using it:

Suggested change

if len(audio.title.strip()) == 0:

if audio.title is None or len(audio.title.strip()) == 0:

DipayanP007 · 2022-01-16T16:14:55Z

Hey @anoadragon453 @Cephian I have made the changes you requested. Review it
Here is a screenshot of the current embed:

Cephian

The code looks and runs good! I made three suggestions, each just for code style. After that, I think we should be ready to merge. Thanks!

Cephian · 2022-01-16T16:45:16Z

main.py

+        str(os.path.join(f"{attachment_directory_filepath}", f"{filename}"))
+    )
+    content = ""
+    # If the tag doesnot exist or is whitespace display the file name only


Suggested change

# If the tag doesnot exist or is whitespace display the file name only

# If the tag does not exist or is whitespace display the file name only

Cephian · 2022-01-16T17:11:16Z

main.py

+    content = ""
+    # If the tag doesnot exist or is whitespace display the file name only
+    # Otherwise display in the format @user: <Artist-tag> - <Title-tag>
+    if audio.artist is not None and len(audio.artist.strip()) != 0:


I missed this in my first review, but using string.isspace() seems better than checking if len(string.split()) == 0

Suggested change

if audio.artist is not None and len(audio.artist.strip()) != 0:

if audio.artist is not None and not audio.artist.isspace():

Cephian · 2022-01-16T17:11:18Z

main.py

+    if audio.title is not None:
+        if len(audio.title.strip()) == 0:
+            filename = path.splitext(filename)[0]
+            content = content + filename.replace("_", " ")
+        else:
+            content = content + f"{str(audio.title)}"
+    # If the title tag doesnot exist but the artist tag exists, display the file name along with artist tag
+    else:
+        filename = path.splitext(filename)[0]
+        content = content + filename.replace("_", " ")


We can remove some duplicated code if we write the if statements like this

Suggested change

if audio.title is not None:

if len(audio.title.strip()) == 0:

filename = path.splitext(filename)[0]

content = content + filename.replace("_", " ")

else:

content = content + f"{str(audio.title)}"

# If the title tag doesnot exist but the artist tag exists, display the file name along with artist tag

else:

filename = path.splitext(filename)[0]

content = content + filename.replace("_", " ")

if audio.title is not None and not audio.title.isspace():

content = content + f"{str(audio.title)}"

# If the title tag does not exist but the artist tag exists, display the file name along with artist tag

else:

filename = path.splitext(filename)[0]

content = content + filename.replace("_", " ")

DipayanP007 · 2022-01-16T17:23:41Z

Hey @Cephian I made the changes requested. 😄

Cephian

Oops! This time it was my mistake, I recommended code without testing it. It turns out text.isspace() returns False on empty strings, so what you had was better! I fixed the two isspace instances as recommendations (and actually tested the code this time!) so with this, the PR should finally be good.

Cephian · 2022-01-16T17:51:03Z

main.py

+    content = ""
+    # If the tag does not exist or is whitespace display the file name only
+    # Otherwise display in the format @user: <Artist-tag> - <Title-tag>
+    if audio.artist is not None and not audio.artist.isspace():


Suggested change

if audio.artist is not None and not audio.artist.isspace():

if audio.artist is not None and audio.artist.strip():

Cephian · 2022-01-16T17:51:36Z

main.py

+    if audio.artist is not None and not audio.artist.isspace():
+        content = content + f"{str(audio.artist)} - "
+
+    if audio.title is not None and not audio.title.isspace():


Suggested change

if audio.title is not None and not audio.title.isspace():

if audio.title is not None and audio.title.strip():

DipayanP007 · 2022-01-16T18:08:45Z

Hey @Cephian did the changes you requested

Cephian

Everything looks good! Thanks so much for your PR!

DipayanP007 and others added 2 commits January 16, 2022 16:14

Added mp3 tags

bfc3b6e

Made some changes to main.py

bd486c4

Updated main.py to pass the lint checks

4d002e4

DipayanP007 added 2 commits January 16, 2022 20:21

Modified the code for lint

a0a9558

Modified the code for lint

f75676d

Cephian requested changes Jan 16, 2022

View reviewed changes

anoadragon453 reviewed Jan 16, 2022

View reviewed changes

Modified main.py according to the changes

0c39b1a

DipayanP007 requested a review from Cephian January 16, 2022 16:15

Cephian requested changes Jan 16, 2022

View reviewed changes

Modified main.py according to the changes

39cf509

DipayanP007 requested a review from Cephian January 16, 2022 17:23

Cephian requested changes Jan 16, 2022

View reviewed changes

Modified main.py according to the changes

6cc5bd3

DipayanP007 requested a review from Cephian January 16, 2022 18:08

Cephian approved these changes Jan 16, 2022

View reviewed changes

Cephian merged commit c508924 into anoadragon453:main Jan 16, 2022

Cephian mentioned this pull request Jan 16, 2022

Read media info from tags #25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solved: Read media info from tags #30

Solved: Read media info from tags #30

DipayanP007 commented Jan 16, 2022

anoadragon453 commented Jan 16, 2022

DipayanP007 commented Jan 16, 2022

DipayanP007 commented Jan 16, 2022

Cephian left a comment •

edited

Cephian Jan 16, 2022

anoadragon453 left a comment

anoadragon453 Jan 16, 2022

anoadragon453 Jan 16, 2022

anoadragon453 Jan 16, 2022

anoadragon453 Jan 16, 2022

anoadragon453 Jan 16, 2022

DipayanP007 commented Jan 16, 2022

Cephian left a comment

Cephian Jan 16, 2022

Cephian Jan 16, 2022

Cephian Jan 16, 2022

DipayanP007 commented Jan 16, 2022

Cephian left a comment

Cephian Jan 16, 2022

Cephian Jan 16, 2022

DipayanP007 commented Jan 16, 2022

Cephian left a comment


		return discord.utils.escape_markdown(filename)
		# If the tag doesnot exist display the file name only

	# If the tag doesnot exist display the file name only
	# If the tag does not exist or only contains whitespace, display the file name only

	if len(audio.title.strip()) == 0:
	if audio.title is None or len(audio.title.strip()) == 0:

	# If the tag doesnot exist or is whitespace display the file name only
	# If the tag does not exist or is whitespace display the file name only

	if audio.artist is not None and len(audio.artist.strip()) != 0:
	if audio.artist is not None and not audio.artist.isspace():

	if audio.artist is not None and not audio.artist.isspace():
	if audio.artist is not None and audio.artist.strip():

	if audio.title is not None and not audio.title.isspace():
	if audio.title is not None and audio.title.strip():

Solved: Read media info from tags #30

Solved: Read media info from tags #30

Conversation

DipayanP007 commented Jan 16, 2022

Modified Code so that:

Screenshots:

anoadragon453 commented Jan 16, 2022

DipayanP007 commented Jan 16, 2022

DipayanP007 commented Jan 16, 2022

Cephian left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 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

DipayanP007 commented Jan 16, 2022

Cephian 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

DipayanP007 commented Jan 16, 2022

Cephian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DipayanP007 commented Jan 16, 2022

Cephian left a comment

Choose a reason for hiding this comment

Cephian left a comment •

edited