Skip to content

Improve regex detection for the drive_sep_replace default#6417

Merged
snejus merged 5 commits intobeetbox:masterfrom
spaceage64:drive-separator-fix
Mar 10, 2026
Merged

Improve regex detection for the drive_sep_replace default#6417
snejus merged 5 commits intobeetbox:masterfrom
spaceage64:drive-separator-fix

Conversation

@spaceage64
Copy link
Contributor

@spaceage64 spaceage64 commented Mar 6, 2026

Description

I imported an album where a track had the name 1:00 AM - Clear and another track named 12:00 AM - Clear (just two examples).

See: Animal Crossing: New Horizons OST

After import, the former was renamed 1_00 AM - Clear, and the latter 12;00 AM - Clear. Notice the inconsistency of how the : was replaced.

I did not make use of the (hidden) drive_sep_replace setting. These were my replace settings:

replace:                        # prevent file name incompatibiliy
  '[\s]'                        : ' '   # standardize whitespace
  '["`‘’“”]'                    : "'"   # standardize quotes
  '[\u002D\u2010-\u2015\u2E3A]' : '-'   # standardize dashes
  '[\u2E3B\uFE58\uFE63\uFF0D]'  : '-'   # standardize dashes
  '[\xAD]'                      : '-'   # standardize dashes
  '[\\\|\/]'                    : ' '   # slashes, pipe > space
  '[:]'                         : ';'   # colon > semicolon
  '[<>]'                        : '-'   # chevrons > dashes
  '[\?\*]'                      : ''    # remove restricted characters
  '[\x00-\x1F\x7F]'             : ''    # remove basic control characters
  '[\x80-\x9F]'                 : ''    # remove extra control characters
  '^\.'                         : ''    # remove leading period
  '\.$'                         : ''    # remove trailing period
  '^\s+'                        : ''    # remove leading space
  '\s+$'                        : ''    # remove trailing space

I found the issue to be too generic regex for drive separator detection. I'm on macOS, so this is irrelevant to me anyway (and I got around it by adding drive_sep_replace: ';' to my settings), but regardless, I think this could be improved.

This PR improves the regex to detect drive separators. Instead of merely looking for any first character followed by a colon (^\w:), we look for a letter, followed by a colon, followed by a backslash instead (^[a-zA-Z]:\\).

The regex logic is solid, but I am not able to test this on a real Windows environment.

Still have to add an entry to the changelog, will do so soon.

Update

Initially this commit failed the MoveTest.test_move_file_with_colon_alt_separator test because it checks the logic using a C:DOS path. So I had to make the logic less restrictive again, not checking for a backslash (^[a-zA-Z]:). I would argue the test itself should be amended (test with C:\DOS instead), but that's not up to me.

As a result, my case of "1:00 AM" being replaced incorrectly is still resolved, but other hypothetical cases like "a:b" would still not be covered due to an arguably incorrect test limiting a more precise regex.

To Do

  • Documentation. (This is an existing feature, just refined logic)
  • Changelog. (Added an entry to the Bugs section)
  • Tests. (I tested the regex, but did not run this on a real Windows setup)

@spaceage64 spaceage64 requested a review from a team as a code owner March 6, 2026 04:24
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

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.

@spaceage64 spaceage64 force-pushed the drive-separator-fix branch 6 times, most recently from 06ee591 to a6951bf Compare March 6, 2026 16:33
@spaceage64
Copy link
Contributor Author

I see I failed one formatting test for the changelog. Can someone advise on this?

@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.42%. Comparing base (80d08ed) to head (fbe9495).
⚠️ Report is 6 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6417   +/-   ##
=======================================
  Coverage   69.42%   69.42%           
=======================================
  Files         141      141           
  Lines       18452    18452           
  Branches     3020     3020           
=======================================
  Hits        12811    12811           
  Misses       5004     5004           
  Partials      637      637           
Files with missing lines Coverage Δ
beets/dbcore/db.py 94.48% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus
Copy link
Member

snejus commented Mar 6, 2026

I see I failed one formatting test for the changelog. Can someone advise on this?

Try running poe format-docs?

@spaceage64 spaceage64 force-pushed the drive-separator-fix branch from a6951bf to 9c8dce8 Compare March 7, 2026 00:13
@spaceage64
Copy link
Contributor Author

I see I failed one formatting test for the changelog. Can someone advise on this?

Try running poe format-docs?

Thanks but have and this error remains. Maybe I'm doing something wrong, but I don't know what. I only added an entry to the changelog, nothing special. No idea why the test would consider the formatting an issue now?

@snejus
Copy link
Member

snejus commented Mar 7, 2026

Ah I see what's happening - you're using a new docstrfmt version.

Run poetry install to sync the dependencies and re-run poe format-docs.

@spaceage64 spaceage64 force-pushed the drive-separator-fix branch from 9c8dce8 to dc3410f Compare March 9, 2026 10:05
@spaceage64 spaceage64 force-pushed the drive-separator-fix branch from dc3410f to 7b26733 Compare March 9, 2026 10:07
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

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

Thanks! We will improve this further later :)

@snejus snejus force-pushed the drive-separator-fix branch from 7b26733 to e14191e Compare March 9, 2026 20:19
@snejus
Copy link
Member

snejus commented Mar 9, 2026

Ah one last thing - we've had a couple of releases since this PR was submitted - would you mind moving your changelog note to the top, under the Unreleased section?

@snejus snejus merged commit 44dc3cd into beetbox:master Mar 10, 2026
14 checks passed
@spaceage64 spaceage64 deleted the drive-separator-fix branch March 10, 2026 11:21
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