Skip to content

pathlib migration: migrate Library.directory to pathlib.Path - #6991

Open
snejus wants to merge 1 commit into
migrate-art-to-pathfrom
migrate-library-directory-path
Open

snejus wants to merge 1 commit into
migrate-art-to-pathfrom
migrate-library-directory-path

Conversation

@snejus

@snejus snejus commented Sep 5, 2026

Copy link
Copy Markdown
Member

Fixes #6967

What changed

  • Library.directory now uses pathlib.Path instead of string/bytes-style directory values.
  • Path ownership is pushed into Library, with small helper APIs like contains_path() and is_in_directory() replacing repeated ancestry checks across the codebase.
  • Callers that still need byte paths for older file-moving logic now convert explicitly at the boundary with os.fsencode(...).

Architecture impact

  • This centralizes library path logic in beets.library.library.Library instead of spreading low-level path checks across importers, UI commands, models, and plugins.
  • The change moves the codebase toward a clearer split:
    • high-level code works with Path objects
    • compatibility code converts only where older path APIs still require bytes
  • Several plugins and commands now use path-native operations like Path.is_dir(), Path.chmod(), Path.stat(), and Path joins, which reduces manual path handling.

Why this matters

  • Makes path handling more consistent and easier to reason about.
  • Reduces duplicated "is this file inside the library?" logic.
  • Creates a cleaner base for future migration away from mixed string/bytes filesystem handling.

High-level impact for reviewers

  • Most call-site changes are mechanical updates from raw path checks to Library helpers or Path methods.
  • The main behavior change is type normalization around Library.directory; file placement and cleanup logic should remain the same.
  • Good review focus areas are duplicate cleanup, move/sync flows, and plugins that manipulate filesystem paths or permissions.

Copilot AI lite review requested due to automatic review settings September 5, 2026 01:34
@snejus
snejus requested a review from a team as a code owner September 5, 2026 01:34
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

grug see real behavior bugs in unimported ignore-subdirectory traversal and permissions directory selection that can touch wrong folders, plus small message/doc fixes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

PR make Library.directory be pathlib.Path at runtime, and push “is this inside library?” logic into Library helper methods. This help reduce mixed bytes/str/path mess across importer, UI, models, and plugins.

Changes:

  • Change Library.directory type to Path, add contains_path() / is_in_directory() helpers, update callers to use them.
  • Update plugins and commands to use Path operations (is_dir(), stat(), chmod(), joins) and only os.fsencode(...) at legacy boundaries.
  • Update tests/helpers to pass Path library directories.
File summaries
File Description
test/test_library.py Switch test setup to Path(...) for lib.directory.
test/plugins/test_permissions.py Update permissions tests to use Path APIs and .filepath fields.
beetsplug/unimported.py Migrate ignore/path logic to Path and Path joins.
beetsplug/permissions.py Migrate permissions plugin to Path-based stat/chmod and path handling.
beetsplug/mbsync.py Replace ancestry check with lib.is_in_directory(...).
beetsplug/ipfs.py Construct temp library with directory=Path("/ipfs/").
beetsplug/bpsync.py Replace ancestry check with lib.is_in_directory(...).
beets/ui/commands/update.py Use lib.is_in_directory(...) and Path.is_dir() for directory check.
beets/ui/init.py Open library using config["directory"].as_path() and log Path values.
beets/test/helper.py Pass Path into Library(...) in test harness.
beets/plugins.py Replace ancestry check with lib.is_in_directory(...).
beets/library/models.py Encode Path directory to bytes at legacy destination/move boundaries.
beets/library/library.py Make directory: Path and add library path helper APIs.
beets/importer/tasks.py Use session.lib.contains_path(...) for in-library checks.
beets/context.py Accept PathLike for music-dir context, encode with os.fsencode.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread beetsplug/permissions.py
Comment thread beetsplug/unimported.py
Comment thread beets/library/library.py Outdated
Comment thread beets/ui/__init__.py Outdated
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.93750% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.92%. Comparing base (21c9299) to head (8e5b774).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/unimported.py 8.33% 11 Missing ⚠️
beets/importer/tasks.py 16.66% 0 Missing and 5 partials ⚠️
beetsplug/permissions.py 84.21% 1 Missing and 2 partials ⚠️
beets/ui/commands/update.py 60.00% 1 Missing and 1 partial ⚠️
beets/library/models.py 66.66% 0 Missing and 1 partial ⚠️
beets/plugins.py 0.00% 0 Missing and 1 partial ⚠️
beetsplug/bpsync.py 50.00% 1 Missing ⚠️
beetsplug/mbsync.py 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##           migrate-art-to-path    #6991      +/-   ##
=======================================================
- Coverage                76.92%   76.92%   -0.01%     
=======================================================
  Files                      163      163              
  Lines                    21729    21734       +5     
  Branches                  3348     3349       +1     
=======================================================
+ Hits                     16716    16719       +3     
- Misses                    4199     4200       +1     
- Partials                   814      815       +1     
Files with missing lines Coverage Δ
beets/context.py 100.00% <100.00%> (ø)
beets/library/library.py 94.00% <100.00%> (+0.31%) ⬆️
beets/ui/__init__.py 82.86% <100.00%> (ø)
beetsplug/ipfs.py 31.55% <100.00%> (+0.30%) ⬆️
beets/library/models.py 87.24% <66.66%> (ø)
beets/plugins.py 89.51% <0.00%> (-0.05%) ⬇️
beetsplug/bpsync.py 25.00% <50.00%> (ø)
beetsplug/mbsync.py 83.52% <50.00%> (ø)
beets/ui/commands/update.py 76.76% <60.00%> (ø)
beetsplug/permissions.py 88.88% <84.21%> (-3.57%) ⬇️
... and 2 more
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus
snejus force-pushed the migrate-library-directory-path branch from 64d10db to a6166c4 Compare September 5, 2026 19:56
@snejus
snejus force-pushed the migrate-library-directory-path branch from a6166c4 to 8e5b774 Compare September 5, 2026 20:03
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