Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to include playlist author in title #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anisjonischkeit
Copy link
Contributor

No description provided.

""",
)
logger.debug(f"Error was {e}")
return None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return None
return "Unknown"

@@ -43,6 +43,19 @@ def get_playlist_tracks(
logger.debug(f"Error fetching tracks from Spotify: {e}")
return tracks

def get_playlist_author(self: "SpotifyClass", playlist_id: str) -> str | None:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def get_playlist_author(self: "SpotifyClass", playlist_id: str) -> str | None:
def get_playlist_author(self: "SpotifyClass", playlist_id: str) -> str:

Comment on lines +88 to +99
if (self.include_playlist_author):
playlist_author: str | None = self.spotify_service.get_playlist_author(
playlist_id,
)

parts = playlist_author.split()
first_name = parts[0]
last_name = parts[-1]
last_initial = last_name[0] # Get the first letter of the last name
short_author = f"{first_name} {last_initial}."

playlist_name = f"{playlist_name} [{short_author}]"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (self.include_playlist_author):
playlist_author: str | None = self.spotify_service.get_playlist_author(
playlist_id,
)
parts = playlist_author.split()
first_name = parts[0]
last_name = parts[-1]
last_initial = last_name[0] # Get the first letter of the last name
short_author = f"{first_name} {last_initial}."
playlist_name = f"{playlist_name} [{short_author}]"
if (self.include_playlist_author):
playlist_author: str = self.spotify_service.get_playlist_author(
playlist_id,
)
if playlist_author is "Unknown":
playlist_name = f"{playlist_name} [{playlist_author}]"
else:
parts = playlist_author.split()
first_name = parts[0]
last_name = parts[-1]
last_initial = last_name[0] # Get the first letter of the last name
short_author = f"{first_name} {last_initial}."
playlist_name = f"{playlist_name} [{short_author}]"

This should make it more adaptable and not cause a failure if we can't pull the name for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point! I'd personally keep get_playlist_author returning None and change the following as shown below:

This way if we can get the playlist author it shows My Playlist Name [Author], or just My Playlist Name if we can't

Suggested change
if (self.include_playlist_author):
playlist_author: str | None = self.spotify_service.get_playlist_author(
playlist_id,
)
parts = playlist_author.split()
first_name = parts[0]
last_name = parts[-1]
last_initial = last_name[0] # Get the first letter of the last name
short_author = f"{first_name} {last_initial}."
playlist_name = f"{playlist_name} [{short_author}]"
if (self.include_playlist_author):
playlist_author: str | None = self.spotify_service.get_playlist_author(
playlist_id,
)
if playlist_author is not None:
parts = playlist_author.split()
first_name = parts[0]
last_name = parts[-1]
last_initial = last_name[0] # Get the first letter of the last name
short_author = f"{first_name} {last_initial}."
playlist_name = f"{playlist_name} [{short_author}]"

Let me know what you think

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better, but users will expect an author if they have the setting enabled, so add a logger.error if the author is none with the playlist name and that should be good I think 🙂

Something like logger.error(f"Unable to get author for {playlist_name})?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense to me. I'll fix that up

Copy link
Owner

@cmathews393 cmathews393 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants