Skip to content

Conversation

@EmberLightVFX
Copy link
Contributor

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

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

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.

Copilot AI review requested due to automatic review settings October 31, 2025 11:07
@EmberLightVFX EmberLightVFX requested a review from a team as a code owner October 31, 2025 11:07
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Contributor

Copilot AI left a 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 SaveSkippedSongsPlugin with configuration options for Spotify integration and output file path
  • Integrates with the import workflow by listening to the import_task_choice event 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.

EmberLightVFX and others added 7 commits October 31, 2025 12:11
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>
Copy link
Member

@henry-oberholtzer henry-oberholtzer left a 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.

Comment on lines +103 to +107
spotify_plugin = None
for plugin in plugins.find_plugins():
if plugin.name == "spotify":
spotify_plugin = plugin
break
Copy link
Member

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.

Copy link
Contributor Author

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"):
Copy link
Member

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?

Copy link
Contributor Author

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?

Comment on lines +24 to +25
- **path**: Path to the file to write the skipped songs to. Default:
``skipped_songs.txt``.
Copy link
Member

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.

Copy link
Contributor Author

@EmberLightVFX EmberLightVFX Nov 1, 2025

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?

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