Skip to content

Conversation

@tommyschnabel
Copy link

Description

Updated the badfiles plugin to support automatic actions taken on warning or error imports, to be used with check_on_import: yes.

Example usage:

badfiles:
    check_on_import: yes
    import_action_on_warning: continue
    import_action_on_error: skip

The above configuration will skip badfiles that error out, but ignore any warnings found.
The default for both options is ask, preserving current behavior.

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.)

@tommyschnabel tommyschnabel requested a review from a team as a code owner January 16, 2026 16:34
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 - I've found 2 issues, and left some high level feedback:

  • The logic for detecting warnings/errors by checking if the words 'warning' or 'error' appear in error_line.lower() is brittle (e.g. localization, different phrasing); consider passing structured severity information from the check instead of inferring it from the message text.
  • The handle_import_action function hardcodes the allowed action strings in multiple places; consider centralizing the set of valid actions (e.g. constant or enum-like structure) to avoid drift and to make configuration validation clearer.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The logic for detecting warnings/errors by checking if the words 'warning' or 'error' appear in `error_line.lower()` is brittle (e.g. localization, different phrasing); consider passing structured severity information from the check instead of inferring it from the message text.
- The `handle_import_action` function hardcodes the allowed action strings in multiple places; consider centralizing the set of valid actions (e.g. constant or enum-like structure) to avoid drift and to make configuration validation clearer.

## Individual Comments

### Comment 1
<location> `beetsplug/badfiles.py:230-232` </location>
<code_context>
+
                     ui.print_(error_line)

+            if found_error and error_action != "ask":
+                return self.handle_import_action(error_action, "error")
+            elif found_warning and warning_action != "ask":
+                return self.handle_import_action(warning_action, "warning")
+
</code_context>

<issue_to_address>
**issue (bug_risk):** Mixed warning/error handling can ignore a stricter warning policy when errors are present.

Given this ordering, when both `found_error` and `found_warning` are true and `error_action != "ask"`, the warning policy is effectively ignored. For instance, with `error_action = "continue"` and `warning_action = "abort"`, execution will continue despite the abort-on-warning configuration. If that’s unintended, consider resolving actions by severity (e.g., assigning priorities and choosing the most conservative) or clearly documenting that error handling always takes precedence over warning handling.
</issue_to_address>

### Comment 2
<location> `docs/plugins/badfiles.rst:37-38` </location>
<code_context>
 and errors will be presented when selecting a tagging option.

+`import_action_on_error` and `import_action_on_warning` can be used to take
+automatic action on warning and errors. Both options default to `ask`.
+Valid options for both `import_action_on_(warning|error)` are
+`ask skip abort continue`.
</code_context>

<issue_to_address>
**issue (typo):** Fix grammar in "warning and errors" phrase.

The phrase is grammatically inconsistent; please update it to "automatic action on warnings and errors."

```suggestion
`import_action_on_error` and `import_action_on_warning` can be used to take
automatic action on warnings and errors. Both options default to `ask`.
```
</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.

@tommyschnabel tommyschnabel force-pushed the badfiles_auto_action branch 2 times, most recently from cc00d7b to 6cfb2c0 Compare January 16, 2026 16:36
@tommyschnabel
Copy link
Author

Sorry for the forced updates, everything is sync'ed with upstream/master now

@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 3.70370% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.69%. Comparing base (4572a7d) to head (fd77422).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/badfiles.py 3.70% 26 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6295      +/-   ##
==========================================
- Coverage   68.78%   68.69%   -0.10%     
==========================================
  Files         140      140              
  Lines       18619    18646      +27     
  Branches     3054     3062       +8     
==========================================
+ Hits        12807    12808       +1     
- Misses       5164     5190      +26     
  Partials      648      648              
Files with missing lines Coverage Δ
beetsplug/badfiles.py 18.23% <3.70%> (-2.98%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tommyschnabel
Copy link
Author

@snejus What's the proper way to request a review?

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.

1 participant