-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New Plugin: Save skipped imports #6140
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Consider caching the set of already-recorded songs in memory to avoid reading the file on every skip and improve performance.
- Before writing to the configured path, ensure that its parent directory exists or create it to avoid write errors in nested directories.
- You can simplify Spotify plugin retrieval by using the official plugin registry (e.g. plugins.get('spotify')) rather than iterating over find_plugins and dynamic imports.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider caching the set of already-recorded songs in memory to avoid reading the file on every skip and improve performance.
- Before writing to the configured path, ensure that its parent directory exists or create it to avoid write errors in nested directories.
- You can simplify Spotify plugin retrieval by using the official plugin registry (e.g. plugins.get('spotify')) rather than iterating over find_plugins and dynamic imports.
## Individual Comments
### Comment 1
<location> `beetsplug/saveskippedsongs.py:63-64` </location>
<code_context>
+ if task.choice_flag == Action.SKIP:
+ # If spotify integration is enabled, try to match with Spotify
+ link = None
+ if self.config["spotify"].get(bool):
+ link = self._match_with_spotify(task, session)
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Boolean config retrieval may be unreliable for non-bool values.
Use `.as_bool()` for config values or ensure they are always boolean to avoid unexpected behavior.
```suggestion
if self.config["spotify"].as_bool():
link = self._match_with_spotify(task, session)
```
</issue_to_address>
### Comment 2
<location> `beetsplug/saveskippedsongs.py:68-70` </location>
<code_context>
+
+ result = f"{summary(task)}{' (' + link + ')' if link else ''}"
+ self._log.info(f"Skipped: {result}")
+ path = self.config["path"].get(str)
+ if path:
+ path = os.path.abspath(path)
</code_context>
<issue_to_address>
**suggestion:** Path config retrieval may not handle user home or environment variables.
Paths containing '~' or environment variables are not expanded. Use `os.path.expanduser` and `os.path.expandvars` to support these cases.
```suggestion
path = self.config["path"].get(str)
if path:
# Expand user home (~) and environment variables in the path
path = os.path.expanduser(os.path.expandvars(path))
path = os.path.abspath(path)
```
</issue_to_address>
### Comment 3
<location> `beetsplug/saveskippedsongs.py:73-81` </location>
<code_context>
+ except FileNotFoundError:
+ existing = set()
+
+ if result not in existing:
+ with open(path, "a", encoding="utf-8") as f:
+ f.write(f"{result}\n")
</code_context>
<issue_to_address>
**suggestion:** Duplicate detection is case-sensitive and ignores whitespace variations.
Normalizing case and whitespace before checking for duplicates will help catch near-duplicates that differ only in formatting.
```suggestion
try:
with open(path, "r", encoding="utf-8") as f:
existing = {
line.rstrip("\n").strip().lower()
for line in f
}
except FileNotFoundError:
existing = set()
normalized_result = result.strip().lower()
if normalized_result not in existing:
with open(path, "a", encoding="utf-8") as f:
f.write(f"{result}\n")
```
</issue_to_address>
### Comment 4
<location> `beetsplug/saveskippedsongs.py:140` </location>
<code_context>
+ )
+
+ # Call the Spotify API directly via the plugin's search method
+ results = spotify_plugin._search_api( # type: ignore[attr-defined]
+ query_type=search_type, # type: ignore[arg-type]
+ query_string=query_string,
</code_context>
<issue_to_address>
**issue:** Direct use of a private method may break with future plugin updates.
Consider using a public method instead, or clearly document the reliance on this internal API.
</issue_to_address>
### Comment 5
<location> `beetsplug/saveskippedsongs.py:42-44` </location>
<code_context>
def summary(task: "ImportTask"):
"""Given an ImportTask, produce a short string identifying the
object.
"""
if task.is_album:
return f"{task.cur_artist} - {task.cur_album}"
else:
item = task.item # type: ignore[attr-defined]
return f"{item.artist} - {item.title}"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
```suggestion
item = task.item # type: ignore[attr-defined]
return f"{item.artist} - {item.title}"
```
</issue_to_address>
### Comment 6
<location> `beetsplug/saveskippedsongs.py:59` </location>
<code_context>
def log_skipped_song(self, task: "ImportTask", session: "ImportSession"):
if task.choice_flag == Action.SKIP:
# If spotify integration is enabled, try to match with Spotify
link = None
if self.config["spotify"].get(bool):
link = self._match_with_spotify(task, session)
result = f"{summary(task)}{' (' + link + ')' if link else ''}"
self._log.info(f"Skipped: {result}")
path = self.config["path"].get(str)
if path:
path = os.path.abspath(path)
try:
# Read existing lines (if file exists) and avoid duplicates.
try:
with open(path, "r", encoding="utf-8") as f:
existing = {line.rstrip("\n") for line in f}
except FileNotFoundError:
existing = set()
if result not in existing:
with open(path, "a", encoding="utf-8") as f:
f.write(f"{result}\n")
else:
self._log.debug(f"Song already recorded in {path}")
except OSError as exc:
# Don't crash import; just log the I/O problem.
self._log.debug(
f"Could not write skipped song to {path}: {exc}"
)
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Add guard clause ([`last-if-guard`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/last-if-guard/))
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Use f-string instead of string concatenation [×2] ([`use-fstring-for-concatenation`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-fstring-for-concatenation/))
</issue_to_address>
### Comment 7
<location> `beetsplug/saveskippedsongs.py:97` </location>
<code_context>
def _match_with_spotify(
self, task: "ImportTask", session: "ImportSession"
) -> Optional[str]:
"""Try to match the skipped track/album with Spotify by directly
calling the Spotify API search.
"""
try:
# Try to get the spotify plugin if it's already loaded
spotify_plugin = None
for plugin in plugins.find_plugins():
if plugin.name == "spotify":
spotify_plugin = plugin
break
# If not loaded, try to load it dynamically
if not spotify_plugin:
try:
from beetsplug.spotify import SpotifyPlugin
spotify_plugin = SpotifyPlugin()
self._log.debug("Loaded Spotify plugin dynamically")
except ImportError as e:
self._log.debug(f"Could not import Spotify plugin: {e}")
return
except Exception as e:
self._log.debug(f"Could not initialize Spotify plugin: {e}")
return
# Build search parameters based on the task
query_filters: SearchFilter = {}
if task.is_album:
query_string = task.cur_album or ""
if task.cur_artist:
query_filters["artist"] = task.cur_artist
search_type = "album"
else:
# For singleton imports
item = task.item # type: ignore[attr-defined]
query_string = item.title or ""
if item.artist:
query_filters["artist"] = item.artist
if item.album:
query_filters["album"] = item.album
search_type = "track"
self._log.info(
f"Searching Spotify for: {query_string} ({query_filters})"
)
# Call the Spotify API directly via the plugin's search method
results = spotify_plugin._search_api( # type: ignore[attr-defined]
query_type=search_type, # type: ignore[arg-type]
query_string=query_string,
filters=query_filters,
)
if results:
self._log.info(f"Found {len(results)} Spotify match(es)!")
self._log.info("Returning first Spotify match link")
return results[0].get("external_urls", {}).get("spotify", "")
else:
self._log.info("No Spotify matches found")
except AttributeError as e:
self._log.debug(f"Spotify plugin method not available: {e}")
except Exception as e:
self._log.debug(f"Error searching Spotify: {e}")
return
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Use the built-in function `next` instead of a for-loop ([`use-next`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-next/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new saveskippedsongs plugin that logs skipped songs/albums during import to a text file for later review. The plugin optionally attempts to find Spotify links for skipped items using the Spotify plugin if available and configured.
- Implements
SaveSkippedSongsPluginwith configuration options for Spotify integration and output file path - Integrates with the import workflow by listening to the
import_task_choiceevent and logging when items are skipped - Adds documentation for the new plugin
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| beetsplug/saveskippedsongs.py | New plugin implementation that saves skipped songs to a text file and optionally searches Spotify for links |
| docs/plugins/saveskippedsongs.rst | Documentation for the new plugin including configuration options |
| docs/changelog.rst | Changelog entry for the new plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
… both track and album
henry-oberholtzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! This looks great so far! It is probably is better to hook into the existing database querying mechanism's results (as I noted in the review) rather than having to re-invent the Spotify API wheel. It'll also make it really easy to increase how useful this is for users who may not user Spotify.
| spotify_plugin = None | ||
| for plugin in plugins.find_plugins(): | ||
| if plugin.name == "spotify": | ||
| spotify_plugin = plugin | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can assume that if a user doesn't have spotify loaded as a plugin, they may not be interested in the spotify feature.
Could also avoid having to make as second API call as well, since by the time the user application or may skip a song, it will have probably already attempted to grab candidates from the import task. It should be available under ImportTask.candidates here, and then it'd just be a member of the AlbumInfo.info or TrackMatch.info object - which should come with the distance already calculated nicely too. Could let the user just filter what database source URLs they wanted printed with it in a config option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably just don't really understand your explenation so if I'm wrong please correct me.
For the addition of the spotify links to be added you would need the spotify plugin to be configured but disabled in your config. If it's active beets will mostly just pick the spotify match as the best match and move on (This is some info I should add to the documentation now when thinking about it).
If the spotify plugin is disabled we would need to do the API call when the user presses skip to see if there is any Spotify matches (without picking it as the beets match)
| __version__ = "1.0" | ||
|
|
||
|
|
||
| def summary(task: "SingletonImportTask"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the more general ImportTask is a better suited typehint than the than the derived SingletonImportTask - I don't think there's any functionality in this plugin that appears to be Singleton only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be if you skipp a singleton song?
| - **path**: Path to the file to write the skipped songs to. Default: | ||
| ``skipped_songs.txt``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to specify that it stores in the user's home directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it will save the file in the dir that I run the beets command from. Should I maybe change the default path to ~/skipped_songs.txt to make it a more specified path?
Description
This plugin will save the name of the skipped song/album to a text file during import for later review.
It will also allow you to try to find the Spotify link for the skipped songs if the Spotify plugin is installed and configured.
This information can later be used together with other MB importers like Harmony.
If any song has already been written to the file, it will not be written again.
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)Extra Info
I haven't written any tests for this plugin as I'm not sure how to write tests for this kind of thing. If anyone knows how to write some good tests for this plugin I'm more than happy to add them to this PR!
I personally wrote this plugin to later use with Harmony as I currently manually save the skipped song-info from my library of not found songs, search spotify for them and add them through Harmony. Currently I have over 100 skipped songs and I'm only on the letter D in my library so this plugin will save me A LOT of time getting the information to later add.