Skip to content

Refactor scanners, add typed models, improve paths, and setup testing…#3

Merged
twardoch merged 1 commit into
mainfrom
feature/modernize-codebase-p1
Jun 23, 2025
Merged

Refactor scanners, add typed models, improve paths, and setup testing…#3
twardoch merged 1 commit into
mainfrom
feature/modernize-codebase-p1

Conversation

@twardoch

@twardoch twardoch commented Jun 23, 2025

Copy link
Copy Markdown
Owner

User description

…/coverage framework

Completed first phase of codebase modernization:

  • Refactored plugin scanning logic into separate AUScanner and VST3Scanner classes within a new 'scanners' module.
  • Introduced dataclasses for PluginInfo and PluginParameter for improved type safety and data consistency.
  • Updated plugin cache loading/saving to use these new models.
  • Enhanced cross-platform support for cache paths (added Linux XDG).
  • Modernized resource loading to use importlib.resources.
  • Added initial unit tests for the new scanner modules and CLI operations.
  • Integrated Codecov for test coverage reporting in CI.
  • Began setup for Sphinx documentation.

PR Type

Enhancement, Tests, Documentation


Description

Major architectural refactor: Split monolithic scanner into modular AUScanner and VST3Scanner classes with improved separation of concerns
Enhanced type safety: Introduced PluginInfo and PluginParameter dataclasses with comprehensive type annotations throughout codebase
Cross-platform improvements: Added Linux XDG cache directory support and modernized resource loading with importlib.resources
Comprehensive testing suite: Added unit tests for CLI commands, scanner modules, and cross-platform functionality with mocked data
CI/CD enhancements: Integrated Codecov for test coverage reporting and added mypy pre-commit hooks for static type checking
Project modernization: Migrated from setup.py to modern pyproject.toml configuration with updated dependencies
Documentation improvements: Added coverage badge and comprehensive codebase representation for AI analysis


Changes walkthrough 📝

Relevant files
Enhancement
8 files
scanner.py
Major refactor of scanner with typed models and modular architecture

src/pedalboard_pluginary/scanner.py

• Refactored plugin scanning logic into separate AUScanner and
VST3Scanner classes
• Added type annotations throughout the module and
improved error handling
• Introduced PluginInfo and PluginParameter
dataclasses for better type safety
• Enhanced plugin parameter
extraction with more robust metadata handling

+287/-155
data.py
Enhanced data handling with cross-platform support and modern resource
loading

src/pedalboard_pluginary/data.py

• Enhanced cross-platform cache path support with Linux XDG compliance

• Modernized resource loading to use importlib.resources with
pkg_resources fallback
• Added PluginInfo reconstruction logic for
loading cached plugin data
• Improved error handling and type
annotations throughout

+150/-26
vst3_scanner.py
New modular VST3 scanner implementation                                   

src/pedalboard_pluginary/scanners/vst3_scanner.py

• Created new VST3Scanner class with platform-specific folder
detection
• Implemented plugin discovery logic with ignore list
support
• Added comprehensive type annotations and logging

+149/-0 
au_scanner.py
New modular Audio Unit scanner for macOS                                 

src/pedalboard_pluginary/scanners/au_scanner.py

• Created new AUScanner class for macOS Audio Unit plugin discovery

Implemented auval output parsing with regex pattern matching
• Added
bundle path resolution logic for AU component files

+157/-0 
__main__.py
Enhanced CLI with verbose logging and type safety               

src/pedalboard_pluginary/main.py

• Enhanced CLI with verbose logging options and better argument
handling
• Added type annotations and improved function naming for
clarity
• Implemented proper logging configuration based on verbosity
levels

+73/-23 
models.py
New typed dataclasses for plugin information                         

src/pedalboard_pluginary/models.py

• Added PluginInfo and PluginParameter dataclasses for type-safe
plugin representation
• Defined ParameterValue type alias for plugin
parameter values
• Included optional fields for manufacturer and
file-specific plugin names

+76/-0   
core.py
Core module type safety improvements                                         

src/pedalboard_pluginary/core.py

• Added type annotations and improved error handling
• Enhanced plugin
data loading with better type checking
• Fixed method call to use
updated scanner interface

+20/-5   
utils.py
Utility functions type safety and boolean parsing improvements

src/pedalboard_pluginary/utils.py

• Added comprehensive type annotations for utility functions

Enhanced from_pb_param function with better boolean parsing logic

Improved parameter value conversion with explicit type handling

+11/-4   
Tests
5 files
test_cli.py
Complete CLI testing suite with mocked plugin data             

tests/test_cli.py

• Added comprehensive unit tests for CLI commands (scan, update, list,
json, yaml)
• Implemented mock data and fixtures for testing plugin
cache operations
• Created helper functions for subprocess-based CLI
testing with proper mocking

+281/-0 
test_au_scanner.py
Unit tests for Audio Unit scanner functionality                   

tests/scanners/test_au_scanner.py

• Added unit tests for AUScanner class covering auval output parsing

Implemented tests for plugin path resolution and bundle detection
logic
• Added platform-specific tests and error condition handling

+236/-0 
test_vst3_scanner.py
Cross-platform VST3 scanner testing suite                               

tests/scanners/test_vst3_scanner.py

• Added comprehensive tests for VST3Scanner across different platforms

• Implemented tests for default folder detection on Windows, macOS,
and Linux
• Added tests for plugin discovery with extra folders and
ignore functionality

+178/-0 
test_data.py
Cross-platform cache path testing                                               

tests/test_data.py

• Added tests for cross-platform cache path generation
• Implemented
platform-specific tests for Windows, macOS, and Linux
• Added XDG
cache directory support testing for Linux

+21/-1   
__init__.py
Test package initialization for scanners                                 

tests/scanners/init.py

• Added package initialization file for scanner tests
• Created empty
init file to establish test package structure

+2/-0     
Miscellaneous
2 files
__init__.py
Simplified imports removing legacy Python compatibility   

src/pedalboard_pluginary/init.py

• Removed Python 3.7 compatibility code for importlib_metadata

Simplified imports to use standard library importlib.metadata directly

+1/-7     
__init__.py
Package initialization for scanners module                             

src/pedalboard_pluginary/scanners/init.py

• Added package initialization file for scanners module
• Created
empty init file to establish scanners as a Python package

+10/-0   
Configuration changes
3 files
pyproject.toml
Complete project configuration modernization                         

pyproject.toml

• Complete project configuration overhaul with modern Python packaging

• Added comprehensive dependencies, testing configuration, and mypy
setup
• Configured pytest with coverage reporting and tool-specific
settings

+95/-3   
.pre-commit-config.yaml
Added mypy pre-commit hook for type checking                         

.pre-commit-config.yaml

• Added mypy pre-commit hook for static type checking
• Configured
additional dependencies for mypy to work with project dependencies

Added mypy configuration pointing to pyproject.toml

+22/-0   
ci.yml
CI enhancement with coverage reporting integration             

.github/workflows/ci.yml

• Integrated pytest-cov for test coverage reporting
• Added Codecov
upload action for coverage reporting
• Enhanced CI pipeline with
coverage tracking

+10/-2   
Documentation
2 files
README.md
Documentation update with coverage badge                                 

README.md

• Added Codecov badge for test coverage visibility
• Enhanced
documentation with coverage reporting information

+4/-0     
llms.txt
Add complete codebase representation for AI analysis         

llms.txt

• Added a comprehensive merged representation of the entire codebase
in a single file
• Contains repository structure, file contents, and
metadata for AI analysis
• Includes all source files, tests,
configuration files, and documentation
• Organized with file
summaries, directory structure, and complete file contents

+1455/-0
Additional files
2 files
setup.cfg +0/-87   
setup.py +0/-21   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • …/coverage framework
    
    Completed first phase of codebase modernization:
    - Refactored plugin scanning logic into separate AUScanner and VST3Scanner classes within a new 'scanners' module.
    - Introduced dataclasses for PluginInfo and PluginParameter for improved type safety and data consistency.
    - Updated plugin cache loading/saving to use these new models.
    - Enhanced cross-platform support for cache paths (added Linux XDG).
    - Modernized resource loading to use importlib.resources.
    - Added initial unit tests for the new scanner modules and CLI operations.
    - Integrated Codecov for test coverage reporting in CI.
    - Began setup for Sphinx documentation.
    @sourcery-ai

    sourcery-ai Bot commented Jun 23, 2025

    Copy link
    Copy Markdown

    Reviewer's Guide

    This PR refactors the core scanning workflow by extracting platform-specific logic into dedicated AUScanner and VST3Scanner classes, introduces typed dataclasses for plugin metadata with robust JSON serialization and reconstruction, enhances cross-platform cache path resolution and resource loading, modernizes the CLI with structured logging and Fire commands, and establishes a first suite of unit tests plus CI coverage reporting.

    Entity relationship diagram for plugin cache and ignores data

    erDiagram
        PLUGIN_CACHE {
            string id PK
            string name
            string path
            string filename
            string plugin_type
            string manufacturer
            string name_in_file
        }
        PLUGIN_PARAMETER {
            string name
            string value
        }
        PLUGIN_CACHE ||--o{ PLUGIN_PARAMETER : has
        IGNORES {
            string plugin_key PK
        }
        IGNORES ||..|| PLUGIN_CACHE : excludes
    
    Loading

    Class diagram for new and refactored scanner and model classes

    classDiagram
        class PedalboardScanner {
            - plugins_path: Path
            - plugins: Dict~str, PluginInfo~
            - ignores_path: Path
            - ignores: Set~str~
            - safe_save: bool
            - au_scanner: Optional~AUScanner~
            - vst3_scanner: VST3Scanner
            + __init__()
            + ensure_ignores()
            + save_plugins()
            + get_plugin_params()
            + scan_single_plugin_file()
            + scan_typed_plugins()
            + scan_aufx_plugins()
            + scan_vst3_plugins()
            + scan_all_plugins()
            + full_scan()
            + rescan()
            + update_scan()
            + get_json()
        }
        class AUScanner {
            - ignores: Set~str~
            + __init__(ignores)
            + find_plugin_files(plugin_paths)
        }
        class VST3Scanner {
            - ignores: Set~str~
            + __init__(ignores)
            + find_plugin_files(extra_folders, plugin_paths)
        }
        class PluginInfo {
            + id: str
            + name: str
            + path: str
            + filename: str
            + plugin_type: str
            + parameters: Dict~str, PluginParameter~
            + manufacturer: Optional~str~
            + name_in_file: Optional~str~
        }
        class PluginParameter {
            + name: str
            + value: float|bool|str
        }
        PedalboardScanner --> AUScanner : uses
        PedalboardScanner --> VST3Scanner : uses
        PedalboardScanner --> PluginInfo : manages
        PluginInfo --> PluginParameter : has
    
    Loading

    Class diagram for CLI and core entry points

    classDiagram
        class PedalboardPluginary {
            - plugins_path: Path
            - plugins: Dict~str, Any~
            + __init__()
            + load_data()
            + list_plugins()
        }
        class __main__ {
            + scan_plugins_cli(extra_folders, verbose)
            + update_plugins_cli(extra_folders, verbose)
            + list_json_cli(verbose)
            + list_yaml_cli(verbose)
            + main_cli()
        }
        PedalboardPluginary <.. __main__ : used by
    
    Loading

    Class diagram for data and utility helpers

    classDiagram
        class data {
            + get_cache_path(cache_name)
            + load_json_file(file_path)
            + save_json_file(data, file_path)
            + load_ignores(ignores_path)
            + save_ignores(ignores, ignores_path)
            + copy_default_ignores(destination_path)
        }
        class utils {
            + ensure_folder(path)
            + from_pb_param(data)
        }
        data ..> utils : uses
    
    Loading

    File-Level Changes

    Change Details Files
    Refactor scanner architecture into modular scanner classes
    • Extract AUFX and VST3 discovery into AUScanner and VST3Scanner modules
    • Update PedalboardScanner to compose these scanners and implement scan_single_plugin_file
    • Replace legacy listing and scanning methods with unified scan_typed_plugins flow
    src/pedalboard_pluginary/scanner.py
    src/pedalboard_pluginary/scanners/au_scanner.py
    src/pedalboard_pluginary/scanners/vst3_scanner.py
    Introduce typed dataclasses for plugin metadata and enhance JSON handling
    • Add PluginInfo and PluginParameter dataclasses in models.py
    • Use dataclasses.asdict in save_plugins and rebuild objects in load_json_file
    • Enhance utils and data modules for robust parameter conversion and serialization
    src/pedalboard_pluginary/models.py
    src/pedalboard_pluginary/data.py
    src/pedalboard_pluginary/utils.py
    Improve cross-platform cache path and resource loading
    • Extend get_cache_path to support Windows APPDATA, macOS Application Support, and Linux XDG_CACHE_HOME
    • Copy default ignores file via importlib.resources with pkg_resources fallback
    src/pedalboard_pluginary/data.py
    Modernize CLI with structured logging and Fire commands
    • Implement setup_logging and verbose flag handling in main.py
    • Rename and align CLI commands (scan, update, list/json/yaml) and fix extra_folders parsing
    • Update pyproject.toml with project metadata, scripts, testing and mypy configuration
    src/pedalboard_pluginary/__main__.py
    pyproject.toml
    Set up testing and CI coverage
    • Add unit tests for data module, scanners, and CLI behaviors
    • Configure pytest, pytest-cov, pre-commit mypy hook
    • Integrate Codecov upload step in GitHub Actions
    tests/
    .github/workflows/ci.yml
    .pre-commit-config.yaml
    pyproject.toml

    Tips and commands

    Interacting with Sourcery

    • Trigger a new review: Comment @sourcery-ai review on the pull request.
    • Continue discussions: Reply directly to Sourcery's review comments.
    • Generate a GitHub issue from a review comment: Ask Sourcery to create an
      issue from a review comment by replying to it. You can also reply to a
      review comment with @sourcery-ai issue to create an issue from it.
    • Generate a pull request title: Write @sourcery-ai anywhere in the pull
      request title to generate a title at any time. You can also comment
      @sourcery-ai title on the pull request to (re-)generate the title at any time.
    • Generate a pull request summary: Write @sourcery-ai summary anywhere in
      the pull request body to generate a PR summary at any time exactly where you
      want it. You can also comment @sourcery-ai summary on the pull request to
      (re-)generate the summary at any time.
    • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
      request to (re-)generate the reviewer's guide at any time.
    • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
      pull request to resolve all Sourcery comments. Useful if you've already
      addressed all the comments and don't want to see them anymore.
    • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
      request to dismiss all existing Sourcery reviews. Especially useful if you
      want to start fresh with a new review - don't forget to comment
      @sourcery-ai review to trigger a new review!

    Customizing Your Experience

    Access your dashboard to:

    • Enable or disable review features such as the Sourcery-generated pull request
      summary, the reviewer's guide, and others.
    • Change the review language.
    • Add, remove or edit custom review instructions.
    • Adjust other review settings.

    Getting Help

    @twardoch twardoch merged commit 4de68c4 into main Jun 23, 2025
    0 of 3 checks passed
    @twardoch twardoch deleted the feature/modernize-codebase-p1 branch June 23, 2025 23:14
    @qodo-code-review

    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Complex Logic

    The refactored scanner contains complex nested logic with multiple try-catch blocks, path resolution, and plugin loading. The update_scan method has particularly intricate logic for comparing existing vs new plugins that should be carefully validated for correctness.

        """Alias for full_scan for backward compatibility or semantic preference."""
        self.full_scan(extra_folders=extra_folders)
    
    def update_scan(self, extra_folders: Optional[List[str]] = None) -> None:
        """Scans for new plugins not already in the cache."""
        logger.info("Starting update scan (looking for new plugins)...")
        if not self.plugins_path.exists():
            logger.info("No existing plugin cache found. Performing full scan instead.")
            self.full_scan(extra_folders=extra_folders)
            return
    
        # Load existing plugins to compare against
        # Type annotation for p ensures mypy knows p is a dict and has 'path' and 'type'
        existing_plugins_data = load_json_file(self.plugins_path)
        if not isinstance(existing_plugins_data, dict): # Should be a dict
             logger.warning("Plugin cache is not a valid dictionary. Performing full scan.")
             self.full_scan(extra_folders=extra_folders)
             return
    
        self.plugins = existing_plugins_data # Start with current cache
    
        # Logic to find only new plugins
        # VST3
        all_found_vst3_paths: Set[Path] = {p.resolve() for p in self._find_vst3_plugins(extra_folders=extra_folders)}
        cached_vst3_paths: Set[Path] = {Path(p_info["path"]).resolve() for p_info in self.plugins.values() if p_info.get("type") == "vst3" and "path" in p_info}
        new_vst3_paths_to_scan: List[Path] = sorted(list(all_found_vst3_paths - cached_vst3_paths))
        if new_vst3_paths_to_scan:
            logger.info(f"Found {len(new_vst3_paths_to_scan)} new VST3 plugin files to scan.")
            self.scan_vst3_plugins(plugin_paths=new_vst3_paths_to_scan) # Scan only these new paths
        else:
            logger.info("No new VST3 plugin files found.")
    
        # AUFX (macOS only)
        if platform.system() == "Darwin":
            all_found_aufx_paths: Set[Path] = {p.resolve() for p in self._find_aufx_plugins()}
            cached_aufx_paths: Set[Path] = {Path(p_info["path"]).resolve() for p_info in self.plugins.values() if p_info.get("type") == "aufx" and "path" in p_info}
            new_aufx_paths_to_scan: List[Path] = sorted(list(all_found_aufx_paths - cached_aufx_paths))
            if new_aufx_paths_to_scan:
                logger.info(f"Found {len(new_aufx_paths_to_scan)} new AUFX plugin files to scan.")
                self.scan_aufx_plugins(plugin_paths=new_aufx_paths_to_scan) # Scan only these
            else:
                logger.info("No new AUFX plugin files found.")
    
        # self.save_plugins() is called within scan_typed_plugins if not safe_save,
        # or after each file if safe_save. If scan_typed_plugins wasn't called (no new plugins),
        # an explicit save here might be redundant but harmless if state hasn't changed.
        # However, if safe_save is False, it's crucial here.
        if not self.safe_save and (new_vst3_paths_to_scan or (platform.system() == "Darwin" and 'new_aufx_paths_to_scan' in locals() and new_aufx_paths_to_scan)):
             self.save_plugins()
        elif not new_vst3_paths_to_scan and not (platform.system() == "Darwin" and 'new_aufx_paths_to_scan' in locals() and new_aufx_paths_to_scan):
             logger.info("No new plugins found in update scan. Cache remains unchanged.")
    
    
        logger.info("Update scan finished.")
    Error Handling

    The load_json_file function has complex reconstruction logic for PluginInfo objects with multiple nested try-catch blocks and fallback mechanisms. Error handling could mask important issues and the function tries to handle too many edge cases in one place.

    if file_path.name == f"{PLUGINS_CACHE_FILENAME_BASE}.json":
        reconstructed_plugins: Dict[str, PluginInfo] = {}
        for plugin_id, plugin_data_dict in raw_data.items():
            if not isinstance(plugin_data_dict, dict):
                continue # Skip malformed entries
    
            param_dicts = plugin_data_dict.get("parameters", {})
            reconstructed_params: Dict[str, PluginParameter] = {}
            if isinstance(param_dicts, dict):
                for param_name, param_data_dict in param_dicts.items():
                    if isinstance(param_data_dict, dict):
                        try:
                            # Ensure all required fields for PluginParameter are present or provide defaults
                            # The 'name' in param_data_dict should match param_name from the key
                            reconstructed_params[param_name] = PluginParameter(
                                name=param_data_dict.get("name", param_name), # Use key as fallback
                                value=param_data_dict.get("value") # Let it be None if missing, handle in PluginParameter if needed
                                # Add other fields like min_value, max_value if they are in models.py and JSON
                            )
                        except TypeError as e: # If 'value' is missing and dataclass requires it
                            # Log error or handle as appropriate
                            print(f"Warning: Could not reconstruct parameter {param_name} for {plugin_id}: {e}")
                            continue # Skip this parameter
    
            # Ensure all required fields for PluginInfo are present
            try:
                reconstructed_plugins[plugin_id] = PluginInfo(
                    id=plugin_data_dict.get("id", plugin_id), # Use key as fallback for id
                    name=plugin_data_dict.get("name", "Unknown Plugin Name"),
                    path=plugin_data_dict.get("path", ""),
                    filename=plugin_data_dict.get("filename", ""),
                    plugin_type=plugin_data_dict.get("plugin_type", "unknown"),
                    parameters=reconstructed_params,
                    manufacturer=plugin_data_dict.get("manufacturer"),
                    name_in_file=plugin_data_dict.get("name_in_file")
                )
            except TypeError as e: # If required fields are missing
                print(f"Warning: Could not reconstruct plugin info for {plugin_id}: {e}")
                continue # Skip this plugin
        return reconstructed_plugins
    else:
        # For other JSON files (e.g., ignores list), return the raw loaded data
        # The type hint Dict[Any, Any] is broad; specific callers might expect List or Dict[str,X]
        return raw_data
    Test Complexity

    The CLI tests use complex mocking strategies with multiple patches and side effects. The test setup creates intricate mock scenarios that may not accurately reflect real-world usage and could be brittle to changes.

    # This mock will replace the actual PedalboardScanner instance or its methods
    @patch('pedalboard_pluginary.scanner.PedalboardScanner.scan_all_plugins')
    @patch('pedalboard_pluginary.scanner.PedalboardScanner.update_scan') # Also mock update_scan
    @patch('pedalboard_pluginary.scanner.PedalboardScanner.save_plugins') # Mock save_plugins
    @patch('pedalboard_pluginary.core.PedalboardPluginary.load_data') # Mock load_data in core
    def run_cli_command(
        cli_args_list,
        mock_core_load_data,
        mock_scanner_save_plugins,
        mock_scanner_update_scan,
        mock_scanner_scan_all,
        expected_exit_code=0
    ):
        """Helper to run CLI commands and capture output."""
    
        # If scan or update is called, make them "create" the mock cache file
        def side_effect_scan_or_update(*args, **kwargs):
            cache_file = get_plugins_cache_file()
            cache_file.parent.mkdir(parents=True, exist_ok=True)
            with open(cache_file, 'w') as f:
                json.dump(MOCK_PLUGIN_DATA, f, indent=4)
            # The actual scan methods in PedalboardScanner don't return anything.
            # They modify self.plugins and then call self.save_plugins.
            # We've mocked save_plugins separately.
    
        mock_scanner_scan_all.side_effect = side_effect_scan_or_update
        mock_scanner_update_scan.side_effect = side_effect_scan_or_update
    
        # Mock load_data to set the plugins attribute on an instance if needed,
        # or simply prevent it from trying to load a real file during list commands
        # if scan hasn't run.
        # For 'list', 'json', 'yaml', the PedalboardPluginary instance will try to load.
        # We can patch load_json_file used by PedalboardPluginary.load_data
    
        base_command = ["pbpluginary"]
        full_command = base_command + cli_args_list
    
        try:
            result = subprocess.run(full_command, capture_output=True, text=True, check=False)
            if result.returncode != expected_exit_code:
                print("STDOUT:", result.stdout)
                print("STDERR:", result.stderr)
            assert result.returncode == expected_exit_code
            return result
        except FileNotFoundError:
            pytest.fail("pbpluginary command not found. Ensure it's installed and in PATH for testing.")

    @sourcery-ai sourcery-ai Bot left a comment

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    Hey @twardoch - I've reviewed your changes - here's some feedback:

    Blocking issues:

    • Found 'importlib.resources', which is a module only available on Python 3.7+. This does not work in lower versions, and therefore is not backwards compatible. Use importlib_resources instead for older Python versions. (link)

    General comments:

    • The entry-point in pyproject.toml still points to the old cli function but you renamed it to main_cli—update the script reference (and any imports) to match the new name so the installed pbpluginary command actually runs.
    • In load_json_file, save_json_file, and copy_default_ignores you use plain print calls for warnings—replace those with logger.warning or logger.error so all runtime diagnostics are captured through the logging framework.
    • You load each plugin twice (once in get_plugin_params and again in scan_single_plugin_file just to fetch metadata); consider refactoring get_plugin_params to return both the parameter dict and the plugin instance so you can reuse it and avoid the extra load.
    Prompt for AI Agents
    Please address the comments from this code review:
    ## Overall Comments
    - The entry-point in pyproject.toml still points to the old `cli` function but you renamed it to `main_cli`—update the script reference (and any imports) to match the new name so the installed `pbpluginary` command actually runs.
    - In `load_json_file`, `save_json_file`, and `copy_default_ignores` you use plain `print` calls for warnings—replace those with `logger.warning` or `logger.error` so all runtime diagnostics are captured through the logging framework.
    - You load each plugin twice (once in `get_plugin_params` and again in `scan_single_plugin_file` just to fetch metadata); consider refactoring `get_plugin_params` to return both the parameter dict and the plugin instance so you can reuse it and avoid the extra load.
    
    ## Individual Comments
    
    ### Comment 1
    <location> `src/pedalboard_pluginary/scanner.py:327` </location>
    <code_context>
    +        # or after each file if safe_save. If scan_typed_plugins wasn't called (no new plugins),
    +        # an explicit save here might be redundant but harmless if state hasn't changed.
    +        # However, if safe_save is False, it's crucial here.
    +        if not self.safe_save and (new_vst3_paths_to_scan or (platform.system() == "Darwin" and 'new_aufx_paths_to_scan' in locals() and new_aufx_paths_to_scan)):
    +             self.save_plugins()
    +        elif not new_vst3_paths_to_scan and not (platform.system() == "Darwin" and 'new_aufx_paths_to_scan' in locals() and new_aufx_paths_to_scan):
    </code_context>
    
    <issue_to_address>
    Use of 'locals()' to check for variable existence is fragile.
    
    Initialize 'new_aufx_paths_to_scan' to None at the start of the function for clearer and more reliable logic.
    
    Suggested implementation:
    
    ```python
                if new_aufx_paths_to_scan:
                    logger.info(f"Found {len(new_aufx_paths_to_scan)} new AUFX plugin files to scan.")
                    self.scan_aufx_plugins(plugin_paths=new_aufx_paths_to_scan) # Scan only these
                else:
                    logger.info("No new AUFX plugin files found.")
    
            # self.save_plugins() is called within scan_typed_plugins if not safe_save,
            # or after each file if safe_save. If scan_typed_plugins wasn't called (no new plugins),
            # an explicit save here might be redundant but harmless if state hasn't changed.
            # However, if safe_save is False, it's crucial here.
            if not self.safe_save and (new_vst3_paths_to_scan or (platform.system() == "Darwin" and new_aufx_paths_to_scan)):
                 self.save_plugins()
            elif not new_vst3_paths_to_scan and not (platform.system() == "Darwin" and new_aufx_paths_to_scan):
                 logger.info("No new plugins found in update scan. Cache remains unchanged.")
    
    ```
    
    At the start of the function (before any assignment or use of `new_aufx_paths_to_scan`), add:
    ```python
    new_aufx_paths_to_scan = None
    ```
    This ensures the variable always exists and the logic is reliable.
    </issue_to_address>
    
    ### Comment 2
    <location> `src/pedalboard_pluginary/data.py:46` </location>
    <code_context>
    +def load_json_file(file_path: Path) -> Dict[Any, Any]: # Or more specific if structure is known
    +    """ Load JSON data from a file. """
    +    if file_path.exists():
    +        with open(file_path, 'r', encoding='utf-8') as file:
    +            try:
    +                return json.load(file)
    </code_context>
    
    <issue_to_address>
    No error handling for file I/O errors.
    
    Catch OSError or IOError when opening files to handle unreadable files or permission issues.
    </issue_to_address>
    
    <suggested_fix>
    <<<<<<< SEARCH
        with open(file_path, 'r', encoding='utf-8') as file:
            try:
                raw_data = json.load(file)
            except json.JSONDecodeError:
                return {} # Return empty dict if JSON is corrupted
    =======
        try:
            with open(file_path, 'r', encoding='utf-8') as file:
                try:
                    raw_data = json.load(file)
                except json.JSONDecodeError:
                    return {} # Return empty dict if JSON is corrupted
        except OSError:
            return {} # Return empty dict if file can't be opened/read
    >>>>>>> REPLACE
    
    </suggested_fix>
    
    ### Comment 3
    <location> `src/pedalboard_pluginary/data.py:64` </location>
    <code_context>
    
    -def save_json_file(data, file_path):
    +    # Check if this is the plugins cache file by its name
    +    if file_path.name == f"{PLUGINS_CACHE_FILENAME_BASE}.json":
    +        reconstructed_plugins: Dict[str, PluginInfo] = {}
    +        for plugin_id, plugin_data_dict in raw_data.items():
    </code_context>
    
    <issue_to_address>
    PluginInfo and PluginParameter reconstruction assumes all required fields are present.
    
    Consider adding a warning or error log when skipping malformed entries to avoid silent data loss.
    </issue_to_address>
    
    ### Comment 4
    <location> `src/pedalboard_pluginary/data.py:111` </location>
    <code_context>
    +
    +def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
    +    """ Save JSON data to a file. """
    +    ensure_folder(file_path) # Ensures parent directory exists
    +    with open(file_path, 'w', encoding='utf-8') as file:
    +        json.dump(data, file, indent=4)
    </code_context>
    
    <issue_to_address>
    ensure_folder is called with a file path, not a directory.
    
    Consider renaming ensure_folder to ensure_parent_folder or updating its docstring to clarify that it creates the parent directory when given a file path.
    
    Suggested implementation:
    
    ```python
    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
        """ Save JSON data to a file. """
        ensure_parent_folder(file_path) # Ensures parent directory exists
        with open(file_path, 'w', encoding='utf-8') as file:
            json.dump(data, file, indent=4)
    
    ```
    
    section.
    
    Here are the code changes:
    
    <file_operations>
    <file_operation operation="edit" file_path="src/pedalboard_pluginary/data.py">
    <<<<<<< SEARCH
    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
        """ Save JSON data to a file. """
        ensure_folder(file_path) # Ensures parent directory exists
        with open(file_path, 'w', encoding='utf-8') as file:
            json.dump(data, file, indent=4)
    =======
    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
        """ Save JSON data to a file. """
        ensure_parent_folder(file_path) # Ensures parent directory exists
        with open(file_path, 'w', encoding='utf-8') as file:
            json.dump(data, file, indent=4)
    >>>>>>> REPLACE
    </file_operation>
    </file_operations>
    
    <additional_changes>
    - If `ensure_folder` is defined in this file, rename its definition to `ensure_parent_folder` and update its docstring to clarify that it creates the parent directory for a file path.
    - If `ensure_folder` is imported from another module, update the import statement to import `ensure_parent_folder` instead.
    - Update any other usages of `ensure_folder` in the codebase to use `ensure_parent_folder` for consistency.
    </issue_to_address>
    
    ## Security Issues
    
    ### Issue 1
    <location> `src/pedalboard_pluginary/data.py:135` </location>
    
    <issue_to_address>
    **security (opengrep-rules.python.lang.compatibility.python37-compatibility-importlib2):** Found 'importlib.resources', which is a module only available on Python 3.7+. This does not work in lower versions, and therefore is not backwards compatible. Use importlib_resources instead for older Python versions.
    
    *Source: opengrep*
    </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.

    # or after each file if safe_save. If scan_typed_plugins wasn't called (no new plugins),
    # an explicit save here might be redundant but harmless if state hasn't changed.
    # However, if safe_save is False, it's crucial here.
    if not self.safe_save and (new_vst3_paths_to_scan or (platform.system() == "Darwin" and 'new_aufx_paths_to_scan' in locals() and new_aufx_paths_to_scan)):

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion: Use of 'locals()' to check for variable existence is fragile.

    Initialize 'new_aufx_paths_to_scan' to None at the start of the function for clearer and more reliable logic.

    Suggested implementation:

                if new_aufx_paths_to_scan:
                    logger.info(f"Found {len(new_aufx_paths_to_scan)} new AUFX plugin files to scan.")
                    self.scan_aufx_plugins(plugin_paths=new_aufx_paths_to_scan) # Scan only these
                else:
                    logger.info("No new AUFX plugin files found.")
    
            # self.save_plugins() is called within scan_typed_plugins if not safe_save,
            # or after each file if safe_save. If scan_typed_plugins wasn't called (no new plugins),
            # an explicit save here might be redundant but harmless if state hasn't changed.
            # However, if safe_save is False, it's crucial here.
            if not self.safe_save and (new_vst3_paths_to_scan or (platform.system() == "Darwin" and new_aufx_paths_to_scan)):
                 self.save_plugins()
            elif not new_vst3_paths_to_scan and not (platform.system() == "Darwin" and new_aufx_paths_to_scan):
                 logger.info("No new plugins found in update scan. Cache remains unchanged.")

    At the start of the function (before any assignment or use of new_aufx_paths_to_scan), add:

    new_aufx_paths_to_scan = None

    This ensures the variable always exists and the logic is reliable.

    Comment on lines +46 to +50
    with open(file_path, 'r', encoding='utf-8') as file:
    try:
    raw_data = json.load(file)
    except json.JSONDecodeError:
    return {} # Return empty dict if JSON is corrupted

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion: No error handling for file I/O errors.

    Catch OSError or IOError when opening files to handle unreadable files or permission issues.

    Suggested change
    with open(file_path, 'r', encoding='utf-8') as file:
    try:
    raw_data = json.load(file)
    except json.JSONDecodeError:
    return {} # Return empty dict if JSON is corrupted
    try:
    with open(file_path, 'r', encoding='utf-8') as file:
    try:
    raw_data = json.load(file)
    except json.JSONDecodeError:
    return {} # Return empty dict if JSON is corrupted
    except OSError:
    return {} # Return empty dict if file can't be opened/read


    def save_json_file(data, file_path):
    # Check if this is the plugins cache file by its name
    if file_path.name == f"{PLUGINS_CACHE_FILENAME_BASE}.json":

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion (bug_risk): PluginInfo and PluginParameter reconstruction assumes all required fields are present.

    Consider adding a warning or error log when skipping malformed entries to avoid silent data loss.

    """ Save JSON data to a file. """
    ensure_folder(file_path)
    with open(file_path, 'w') as file:
    ensure_folder(file_path) # Ensures parent directory exists

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion: ensure_folder is called with a file path, not a directory.

    Consider renaming ensure_folder to ensure_parent_folder or updating its docstring to clarify that it creates the parent directory when given a file path.

    Suggested implementation:

    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
        """ Save JSON data to a file. """
        ensure_parent_folder(file_path) # Ensures parent directory exists
        with open(file_path, 'w', encoding='utf-8') as file:
            json.dump(data, file, indent=4)

    section.

    Here are the code changes:

    <file_operations>
    <file_operation operation="edit" file_path="src/pedalboard_pluginary/data.py">
    <<<<<<< SEARCH
    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
    """ Save JSON data to a file. """
    ensure_folder(file_path) # Ensures parent directory exists
    with open(file_path, 'w', encoding='utf-8') as file:
    json.dump(data, file, indent=4)

    def save_json_file(data: Dict[Any, Any], file_path: Path) -> None: # Or list/Any for data
    """ Save JSON data to a file. """
    ensure_parent_folder(file_path) # Ensures parent directory exists
    with open(file_path, 'w', encoding='utf-8') as file:
    json.dump(data, file, indent=4)

    REPLACE
    </file_operation>
    </file_operations>

    <additional_changes>

    • If ensure_folder is defined in this file, rename its definition to ensure_parent_folder and update its docstring to clarify that it creates the parent directory for a file path.
    • If ensure_folder is imported from another module, update the import statement to import ensure_parent_folder instead.
    • Update any other usages of ensure_folder in the codebase to use ensure_parent_folder for consistency.

    # 'pedalboard_pluginary.resources' should be where the resources package is.
    # Or, if 'resources' is a sub-package of 'data's package: from . import resources
    # Assuming 'pedalboard_pluginary' is the top-level package for resources.
    import importlib.resources

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    security (opengrep-rules.python.lang.compatibility.python37-compatibility-importlib2): Found 'importlib.resources', which is a module only available on Python 3.7+. This does not work in lower versions, and therefore is not backwards compatible. Use importlib_resources instead for older Python versions.

    Source: opengrep

    logger.error(f"Error running auval (is it installed and in PATH?): {e}")
    return []

    def find_plugin_files(self, plugin_paths: Optional[List[Path]] = None) -> List[Path]:

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    issue (code-quality): We've found these issues:


    Explanation

    The quality score for this function is below the quality threshold of 25%.
    This score is a combination of the method length, cognitive complexity and working memory.

    How can you solve this?

    It might be worth refactoring this function to make it shorter and more readable.

    • Reduce the function length by extracting pieces of functionality out into
      their own functions. This is the most important thing you can do - ideally a
      function should be less than 10 lines.
    • Reduce nesting, perhaps by introducing guard clauses to return early.
    • Ensure that variables are tightly scoped, so that code using related concepts
      sits together within the function rather than being scattered.

    Comment on lines +20 to 22
    if drep.lower() == "false":
    return False
    return drep

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion (code-quality): We've found these issues:

    Suggested change
    if drep.lower() == "false":
    return False
    return drep
    return False if drep.lower() == "false" else drep

    Comment thread tests/test_cli.py
    Comment on lines +160 to +163
    if path_arg == cache_file:
    # Return raw dict, PedalboardPluginary.load_data will handle reconstruction
    return MOCK_PLUGIN_DATA
    return {} # Default for other calls

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion (code-quality): We've found these issues:

    Suggested change
    if path_arg == cache_file:
    # Return raw dict, PedalboardPluginary.load_data will handle reconstruction
    return MOCK_PLUGIN_DATA
    return {} # Default for other calls
    return MOCK_PLUGIN_DATA if path_arg == cache_file else {}

    Comment thread tests/test_cli.py
    Comment on lines +202 to +204
    if path_arg == cache_file:
    return MOCK_PLUGIN_DATA
    return {}

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion (code-quality): We've found these issues:

    Suggested change
    if path_arg == cache_file:
    return MOCK_PLUGIN_DATA
    return {}
    return MOCK_PLUGIN_DATA if path_arg == cache_file else {}

    Comment thread tests/test_cli.py
    Comment on lines +224 to +226
    if path_arg == cache_file:
    return MOCK_PLUGIN_DATA
    return {}

    Copy link
    Copy Markdown

    Choose a reason for hiding this comment

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

    suggestion (code-quality): We've found these issues:

    Suggested change
    if path_arg == cache_file:
    return MOCK_PLUGIN_DATA
    return {}
    return MOCK_PLUGIN_DATA if path_arg == cache_file else {}

    @qodo-code-review

    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant