Fixes for linux and yabridge#6
Open
scottmudge wants to merge 1 commit into
Open
Conversation
Reviewer's GuideUpdates plugin scanning and cache handling for better Linux and yabridge support, adds a proper CLI entry point, and renames an internal types module to avoid conflicts with Python’s built‑in types. Sequence diagram for updated VST3 and yabridge scanningsequenceDiagram
participant ScannerIsolated
participant FileSystem
ScannerIsolated->>ScannerIsolated: _find_plugins_to_scan(extra_folders)
ScannerIsolated->>ScannerIsolated: processed_stems = empty set
ScannerIsolated->>ScannerIsolated: _get_vst3_folders(extra_folders)
ScannerIsolated->>FileSystem: rglob("*.vst3") on each VST3 folder
loop for each plugin_path
ScannerIsolated->>ScannerIsolated: check plugin_path.stem in processed_stems
alt stem already processed
ScannerIsolated->>ScannerIsolated: optionally log Skipping duplicate
else stem not processed
ScannerIsolated->>ScannerIsolated: check "x86_64-win" in plugin_path.parts
alt path contains x86_64-win
ScannerIsolated->>ScannerIsolated: optionally log Skipping Windows symlink
else not a Windows symlink
ScannerIsolated->>ScannerIsolated: check f"vst3/{plugin_path.stem}" in ignores
alt ignored
ScannerIsolated->>ScannerIsolated: continue loop
else not ignored
ScannerIsolated->>FileSystem: plugin_path.is_dir()
alt plugin_path is directory
ScannerIsolated->>ScannerIsolated: add (path, stem, vst3) to plugin_tasks
ScannerIsolated->>ScannerIsolated: add stem to processed_stems
ScannerIsolated->>ScannerIsolated: optionally log Found VST3 bundle
else plugin_path is file
ScannerIsolated->>FileSystem: plugin_path.is_file()
alt plugin_path is file
ScannerIsolated->>ScannerIsolated: add (path, stem, vst3) to plugin_tasks
ScannerIsolated->>ScannerIsolated: add stem to processed_stems
ScannerIsolated->>ScannerIsolated: optionally log Found VST3 file
end
end
end
end
end
end
ScannerIsolated-->>ScannerIsolated: return plugin_tasks
Sequence diagram for CLI entry point using main functionsequenceDiagram
actor User
participant Shell
participant PythonInterpreter
participant PedalboardMain as pedalboard_pluginary___main__
participant Cli as cli_function
User->>Shell: run pedalboard_pluginary command
Shell->>PythonInterpreter: invoke console script entry point
PythonInterpreter->>PedalboardMain: import pedalboard_pluginary.__main__
PythonInterpreter->>PedalboardMain: if __name__ == __main__
PedalboardMain->>PedalboardMain: main()
PedalboardMain->>Cli: cli()
Cli-->>User: execute CLI logic and output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Review Summary by QodoFix Linux cache paths and add yabridge VST3 plugin support
WalkthroughsDescription• Fixed Linux cache path to use ~/.config/ instead of incorrect location • Added recursive scanning for VST3 plugins to support yabridge directory structure • Renamed types.py to type_definitions.py to avoid shadowing built-in module • Improved plugin deduplication and Windows symlink filtering during scanning • Added cache folder creation and better OS detection logic • Added dependencies for rich and click libraries Diagramflowchart LR
A["Linux Cache Path Fix"] --> B["Use ~/.config/ directory"]
C["Yabridge Support"] --> D["Recursive VST3 scanning"]
D --> E["Handle .vst3 directories"]
E --> F["Skip Windows symlinks"]
G["Module Shadowing Fix"] --> H["Rename types.py to type_definitions.py"]
I["Plugin Deduplication"] --> J["Track processed stems"]
K["OS Detection"] --> L["Improve platform checks"]
L --> M["Create cache folders"]
File Changes1. src/pedalboard_pluginary/__main__.py
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The new
get_cache_pathOS detection mixesos.nameandsys.platformwith some unreachable branches (e.g.os.name == 'mac') and raises aValueErrorwith theosmodule instead ofos.name/system_os; consider simplifying to asys.platform-based check and fixing the error message to reference the actual platform string. - Using
processed_stemsto deduplicate VST3 plugins byplugin_path.stemmay unintentionally drop distinct plugins that share the same name in different directories; consider including more of the path (e.g. parent directory) in the deduplication key if that scenario is possible in real setups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `get_cache_path` OS detection mixes `os.name` and `sys.platform` with some unreachable branches (e.g. `os.name == 'mac'`) and raises a `ValueError` with the `os` module instead of `os.name`/`system_os`; consider simplifying to a `sys.platform`-based check and fixing the error message to reference the actual platform string.
- Using `processed_stems` to deduplicate VST3 plugins by `plugin_path.stem` may unintentionally drop distinct plugins that share the same name in different directories; consider including more of the path (e.g. parent directory) in the deduplication key if that scenario is possible in real setups.
## Individual Comments
### Comment 1
<location> `src/pedalboard_pluginary/scanner_isolated.py:299-305` </location>
<code_context>
) -> set[tuple[str, str, str]]:
"""Find all VST3 and AU plugins and return a set of (path, name, type)."""
plugin_tasks = set()
+ processed_stems = set() # Track plugin names to avoid duplicates
- # VST3
+ # VST3 - use rglob to recursively find plugins (supports yabridge)
for folder in self._get_vst3_folders(extra_folders):
- for plugin_path in folder.glob("*.vst3"):
- if f"vst3/{plugin_path.stem}" not in self.ignores:
- # For VST3, we often have multiple plugins in one file.
- # We will treat the file path as the initial task.
+ for plugin_path in folder.rglob("*.vst3"):
+ # Skip if we've already processed a plugin with this name
+ if plugin_path.stem in processed_stems:
+ if self.verbose:
+ logger.debug(f"Skipping duplicate: {plugin_path}")
</code_context>
<issue_to_address>
**issue:** Deduplicating solely by stem can hide distinct plugins that share the same filename in different folders.
This will skip any later plugin sharing the same basename, even when it’s a different plugin in another folder. If that behavior isn’t intentional, consider including the parent directory in the key (e.g. `(plugin_path.parent, plugin_path.stem)`) or limiting this deduplication to known yabridge layout patterns instead of applying it globally.
</issue_to_address>
### Comment 2
<location> `src/pedalboard_pluginary/data.py:17-22` </location>
<code_context>
def get_cache_path(cache_name):
"""Get the path to a cache file."""
- if os.name == "nt":
+ system_os = sys.platform
+ if os.name == "nt" or system_os == "win32":
cache_folder = Path(os.getenv("APPDATA")) / APP_NAME
- else:
+ elif os.name == "posix" or system_os == "linux":
+ cache_folder = Path.home() / ".config" / APP_NAME
+ elif os.name == "mac" or system_os == "darwin":
cache_folder = Path.home() / "Library" / "Application Support" / APP_NAME
+ else:
</code_context>
<issue_to_address>
**suggestion:** OS detection is redundant and partially incorrect (os.name never equals 'mac').
`os.name` and `sys.platform` are being mixed in a way that’s both redundant and partly incorrect: `os.name` is `'posix'` for both Linux and macOS, so `os.name == "mac"` can never match. Since you already rely on `sys.platform`, consider using only that (e.g. `startswith("linux")`, `== "darwin"`, `startswith("win")`) and removing the `os.name` checks to keep the logic simpler and less error-prone.
</issue_to_address>
### Comment 3
<location> `src/pedalboard_pluginary/data.py:18-25` </location>
<code_context>
+ system_os = sys.platform
+ if os.name == "nt" or system_os == "win32":
cache_folder = Path(os.getenv("APPDATA")) / APP_NAME
- else:
+ elif os.name == "posix" or system_os == "linux":
+ cache_folder = Path.home() / ".config" / APP_NAME
+ elif os.name == "mac" or system_os == "darwin":
cache_folder = Path.home() / "Library" / "Application Support" / APP_NAME
+ else:
+ raise ValueError(f"Unsupported OS: {os}")
+ if not os.path.exists(cache_folder):
+ os.makedirs(cache_folder, exist_ok=True)
</code_context>
<issue_to_address>
**issue (bug_risk):** The ValueError message references the os module instead of the actual OS identifier.
Here `os` refers to the imported module, so the exception will show something like `<module 'os' ...>` rather than the actual platform. Use `os.name`, `system_os`, or both in the message so it clearly reports the unsupported OS value.
</issue_to_address>
### Comment 4
<location> `src/pedalboard_pluginary/data.py:26-27` </location>
<code_context>
cache_folder = Path.home() / "Library" / "Application Support" / APP_NAME
+ else:
+ raise ValueError(f"Unsupported OS: {os}")
+ if not os.path.exists(cache_folder):
+ os.makedirs(cache_folder, exist_ok=True)
# Don't append .json anymore - let the caller specify the full filename
return cache_folder / cache_name
</code_context>
<issue_to_address>
**suggestion:** Mixing pathlib Paths with os.path/os.makedirs is a bit inconsistent and can be simplified.
Since `cache_folder` is already a `Path`, you can replace the `os.path.exists`/`os.makedirs` block with `cache_folder.mkdir(parents=True, exist_ok=True)`. This keeps everything in the pathlib API and removes the small race window between checking existence and creating the directory.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+299
to
+305
| processed_stems = set() # Track plugin names to avoid duplicates | ||
|
|
||
| # VST3 | ||
| # VST3 - use rglob to recursively find plugins (supports yabridge) | ||
| for folder in self._get_vst3_folders(extra_folders): | ||
| for plugin_path in folder.glob("*.vst3"): | ||
| if f"vst3/{plugin_path.stem}" not in self.ignores: | ||
| # For VST3, we often have multiple plugins in one file. | ||
| # We will treat the file path as the initial task. | ||
| for plugin_path in folder.rglob("*.vst3"): | ||
| # Skip if we've already processed a plugin with this name | ||
| if plugin_path.stem in processed_stems: |
There was a problem hiding this comment.
issue: Deduplicating solely by stem can hide distinct plugins that share the same filename in different folders.
This will skip any later plugin sharing the same basename, even when it’s a different plugin in another folder. If that behavior isn’t intentional, consider including the parent directory in the key (e.g. (plugin_path.parent, plugin_path.stem)) or limiting this deduplication to known yabridge layout patterns instead of applying it globally.
Comment on lines
+17
to
+22
| system_os = sys.platform | ||
| if os.name == "nt" or system_os == "win32": | ||
| cache_folder = Path(os.getenv("APPDATA")) / APP_NAME | ||
| else: | ||
| elif os.name == "posix" or system_os == "linux": | ||
| cache_folder = Path.home() / ".config" / APP_NAME | ||
| elif os.name == "mac" or system_os == "darwin": |
There was a problem hiding this comment.
suggestion: OS detection is redundant and partially incorrect (os.name never equals 'mac').
os.name and sys.platform are being mixed in a way that’s both redundant and partly incorrect: os.name is 'posix' for both Linux and macOS, so os.name == "mac" can never match. Since you already rely on sys.platform, consider using only that (e.g. startswith("linux"), == "darwin", startswith("win")) and removing the os.name checks to keep the logic simpler and less error-prone.
Comment on lines
+18
to
+25
| if os.name == "nt" or system_os == "win32": | ||
| cache_folder = Path(os.getenv("APPDATA")) / APP_NAME | ||
| else: | ||
| elif os.name == "posix" or system_os == "linux": | ||
| cache_folder = Path.home() / ".config" / APP_NAME | ||
| elif os.name == "mac" or system_os == "darwin": | ||
| cache_folder = Path.home() / "Library" / "Application Support" / APP_NAME | ||
| else: | ||
| raise ValueError(f"Unsupported OS: {os}") |
There was a problem hiding this comment.
issue (bug_risk): The ValueError message references the os module instead of the actual OS identifier.
Here os refers to the imported module, so the exception will show something like <module 'os' ...> rather than the actual platform. Use os.name, system_os, or both in the message so it clearly reports the unsupported OS value.
Comment on lines
+26
to
+27
| if not os.path.exists(cache_folder): | ||
| os.makedirs(cache_folder, exist_ok=True) |
There was a problem hiding this comment.
suggestion: Mixing pathlib Paths with os.path/os.makedirs is a bit inconsistent and can be simplified.
Since cache_folder is already a Path, you can replace the os.path.exists/os.makedirs block with cache_folder.mkdir(parents=True, exist_ok=True). This keeps everything in the pathlib API and removes the small race window between checking existence and creating the directory.
Code Review by Qodo
1. get_cache_path() OS detection wrong
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+17
to
+27
| system_os = sys.platform | ||
| if os.name == "nt" or system_os == "win32": | ||
| cache_folder = Path(os.getenv("APPDATA")) / APP_NAME | ||
| else: | ||
| elif os.name == "posix" or system_os == "linux": | ||
| cache_folder = Path.home() / ".config" / APP_NAME | ||
| elif os.name == "mac" or system_os == "darwin": | ||
| cache_folder = Path.home() / "Library" / "Application Support" / APP_NAME | ||
| else: | ||
| raise ValueError(f"Unsupported OS: {os}") | ||
| if not os.path.exists(cache_folder): | ||
| os.makedirs(cache_folder, exist_ok=True) |
There was a problem hiding this comment.
1. get_cache_path() os detection wrong 📘 Rule violation ⛯ Reliability
get_cache_path() treats any os.name == 'posix' system as Linux, which will incorrectly route
macOS to the Linux cache path. It also raises ValueError(f"Unsupported OS: {os}"), which provides
misleading context and may expose internal module details.
Agent Prompt
## Issue description
`get_cache_path()` OS detection is incorrect for macOS because it matches `os.name == "posix"` in the Linux branch, and the unsupported-OS error message uses the `os` module object rather than OS identifiers.
## Issue Context
This PR aims to fix Linux cache paths; incorrect branching can regress macOS behavior and create confusing failures.
## Fix Focus Areas
- src/pedalboard_pluginary/data.py[15-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR includes some fixes for Linux (not setting the correct cache path in
~/.config/...), and to allow scanning recursively into the~/.vst3/and allow scanning/loading yabridge plugins, which utilize a directory named ".vst3" rather than a distinct file.It seems to work as expected. Scanning correctly finds the yabridge plugins, and yabridge is correctly invoked by pedalboard during scanning.
Tested and working with Python 3.14.2.
I also renamed the local types.py file, as it was shadowing the built-in
typesmodule, causing collisions/issues.Summary by Sourcery
Improve plugin discovery and cache handling across platforms, with better support for Linux and yabridge-based VST3 plugins.
New Features:
Bug Fixes:
Enhancements:
Build: