Conversation
198bd13 to
1b4eac7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
docs has broken reST inline literal wrapping and limit validation error text is inconsistent for long-only --limit usage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
grug see PR make --limit work everywhere query happen. grug like: one place parse/validate limit, then pass limit=opts.limit down into lib.items() / lib.albums(), so commands + plugins all behave same.
Changes:
- Add shared
CommonOptionsParser.add_limit_option()with validation for non-negative integer limits. - Wire
limit=opts.limitthrough core query commands and many plugins (items + albums paths). - Update tests + docs/changelog so CLI help and plugin docs match new behavior.
File summaries
| File | Description |
|---|---|
| test/ui/test_ui.py | add parser-level tests for --limit validation + custom flags |
| test/ui/commands/test_list.py | remove per-command negative-limit test now covered centrally |
| test/plugins/test_tidal.py | assert tidal plugin passes limit through to library queries |
| docs/reference/cli.rst | document --limit across query commands and flag conflict guidance |
| docs/plugins/zero.rst | document zero supports -l/--limit |
| docs/plugins/tidal.rst | document tidalsync supports -l/--limit |
| docs/plugins/thumbnails.rst | document thumbnails query limiting |
| docs/plugins/spotify.rst | document spotifysync query limiting |
| docs/plugins/replaygain.rst | document replaygain query limiting |
| docs/plugins/replace.rst | document replace query limiting |
| docs/plugins/random.rst | document random plugin limit option |
| docs/plugins/play.rst | document play plugin limit option |
| docs/plugins/missing.rst | document missing plugin limit behavior |
| docs/plugins/metasync.rst | document metasync supports limiting |
| docs/plugins/mbsync.rst | document mbsync supports limiting |
| docs/plugins/mbsubmit.rst | document mbsubmit supports limiting |
| docs/plugins/lyrics.rst | document lyrics uses --limit (no -l conflict) |
| docs/plugins/limit.rst | update deprecation guidance now that more commands have built-in limit |
| docs/plugins/keyfinder.rst | document keyfinder supports limiting |
| docs/plugins/ipfs.rst | document ipfs uses --limit due to -l/--list conflict |
| docs/plugins/info.rst | document info uses --limit due to -l/--library conflict |
| docs/plugins/ftintitle.rst | document ftintitle supports limiting |
| docs/plugins/fetchart.rst | document fetchart supports limiting |
| docs/plugins/export.rst | document export uses --limit due to -l/--library conflict |
| docs/plugins/embedart.rst | document embedart/extractart/clearart support limiting |
| docs/plugins/duplicates.rst | document duplicates supports limiting |
| docs/plugins/deezer.rst | document deezerupdate supports limiting |
| docs/plugins/convert.rst | document convert uses --limit due to -l/--link conflict |
| docs/plugins/chroma.rst | document chroma commands support limiting |
| docs/plugins/bpsync.rst | document bpsync supports limiting |
| docs/plugins/bpm.rst | document bpm supports limiting |
| docs/plugins/bareasc.rst | document bareasc supports limiting |
| docs/plugins/badfiles.rst | document badfiles supports limiting |
| docs/plugins/autobpm.rst | document autobpm supports limiting |
| docs/plugins/acousticbrainz.rst | document acousticbrainz supports limiting |
| docs/plugins/absubmit.rst | document absubmit supports limiting |
| docs/dev/plugins/commands.rst | document new CommonOptionsParser --limit helper for plugin devs |
| docs/changelog.rst | add unreleased changelog entry for expanded --limit support |
| beetsplug/zero.py | add limit option + pass limit into lib.items() |
| beetsplug/titlecase.py | add limit option + pass limit into lib.items() |
| beetsplug/tidal/init.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/thumbnails.py | add limit option + pass limit into lib.albums() |
| beetsplug/spotify.py | add limit option + pass limit into lib.items() |
| beetsplug/scrub.py | add limit option + pass limit into lib.items() |
| beetsplug/replaygain.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/replace.py | add limit option + pass limit into lib.items() |
| beetsplug/random.py | add limit option + limit query before random selection |
| beetsplug/play.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/parentwork.py | add limit option + pass limit into lib.items() |
| beetsplug/missing.py | add limit option + thread limit through missing helpers |
| beetsplug/metasync/init.py | add limit option + pass limit into lib.items() |
| beetsplug/mbsync.py | add limit option + pass limit into singleton/album queries |
| beetsplug/mbsubmit.py | add limit option + pass limit into lib.items() |
| beetsplug/lyrics.py | add --limit only + pass limit into lib.items() |
| beetsplug/limit.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/keyfinder.py | add limit option + pass limit into lib.items() |
| beetsplug/ipfs.py | add --limit only + pass limit into remote album queries + play opts |
| beetsplug/info.py | add --limit only + thread limit into collectors |
| beetsplug/ftintitle.py | add limit option + pass limit into lib.items() |
| beetsplug/fetchart.py | add limit option + pass limit into lib.albums() |
| beetsplug/export.py | add --limit only + thread limit into collectors |
| beetsplug/embedart.py | add limit option + pass limit into lib.items() / lib.albums() + art clear |
| beetsplug/duplicates.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/deezer.py | add limit option + pass limit into lib.items() |
| beetsplug/convert.py | add --limit only + pass limit into lib.items() / lib.albums() |
| beetsplug/chroma.py | add limit option + pass limit into lib.items() across subcommands |
| beetsplug/bpsync.py | add limit option + pass limit into singleton/album queries |
| beetsplug/bpm.py | add limit option + pass limit into lib.items() |
| beetsplug/bench.py | add limit option + limit album pick used for benchmark seed |
| beetsplug/bareasc.py | add limit option + pass limit into lib.items() / lib.albums() |
| beetsplug/badfiles.py | add limit option + pass limit into lib.items() |
| beetsplug/autobpm.py | add limit option + pass limit into lib.items() |
| beetsplug/acousticbrainz.py | add limit option + pass limit into lib.items() |
| beetsplug/absubmit.py | add limit option + pass limit into lib.items() |
| beetsplug/_utils/art.py | add limit param to art.clear() and apply to query |
| beets/ui/commands/write.py | add limit option + pass limit into write query |
| beets/ui/commands/update.py | add limit option + pass limit into update selection |
| beets/ui/commands/stats.py | add limit option + pass limit into stats query |
| beets/ui/commands/remove.py | add limit option + apply to item/album selection |
| beets/ui/commands/move.py | add limit option + apply to item/album selection |
| beets/ui/commands/modify.py | add limit option + apply to item/album selection |
| beets/ui/commands/list.py | use shared limit option + pass limit into queries |
| beets/ui/init.py | implement shared add_limit_option() + central validation |
Review details
- Files reviewed: 83/83 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7003 +/- ##
==========================================
+ Coverage 77.24% 77.42% +0.17%
==========================================
Files 163 163
Lines 21840 21923 +83
Branches 3370 3370
==========================================
+ Hits 16871 16973 +102
+ Misses 4149 4125 -24
- Partials 820 825 +5
🚀 New features to boost your workflow:
|
1b4eac7 to
ac43b8d
Compare
There was a problem hiding this comment.
I started with the review and think we should probably be a bit more selective about where we add the limit option.
For some of these commands, a limit doesn’t really make sense imo, especially for commands that write/update based on a query, since there’s currently no way to continue the incremental update.
For example, why would I only update/recompute BPM values for the first 100 items matching a query and skip everything else? Without an offset or some other pagination mechanism, the limit feels a bit arbitrary and not particularly useful in some cases.
I’d probably add the limit option only to queries where we’re actually displaying/returning information, or where limiting the results genuinely makes sense. 😅
@beetbox/maintainers What do you think?
| dest="exclude_fields", | ||
| help="list of fields to exclude from updates", | ||
| ) | ||
| update_cmd.parser.add_limit_option() |
There was a problem hiding this comment.
I dont think we should have a limit option for update. Seems a bit strange as I'm not sure how to even interpret this 😅
There was a problem hiding this comment.
I think it is useful. Update albums added in the last week.
There was a problem hiding this comment.
You wouldn't do this using a limit tho but with a query based on added 🤔
Doesnt something like this work?
beet update 'added:-1w..'
There was a problem hiding this comment.
Agree with @arsaboo, I do often want to update just a couple of tracks, and often use a query like path::aaa
| default=False, | ||
| help="write tags even if the existing tags match the database", | ||
| ) | ||
| write_cmd.parser.add_limit_option() |
There was a problem hiding this comment.
Generally I don't think we should have limit options for commands that write or update based on a query. This seems a bit like a footgun for users to me.
There was a problem hiding this comment.
I mean, this command accepts a query, so why not provide a limit option here?
| help="comma-separated list of sources to sync", | ||
| ) | ||
| cmd.parser.add_format_option() | ||
| cmd.parser.add_limit_option() |
There was a problem hiding this comment.
I do think we should not add the limit option here either.
| class TidalSyncCLIOpts(Protocol): | ||
| album: bool | ||
| force: bool | ||
| limit: int | None |
There was a problem hiding this comment.
Same here as this updates the library a limit seems like a strange choice.
There was a problem hiding this comment.
I use this routinely (though with Spotify). Limit spotifysync to 3000 to avoid exhausting API limits.
There was a problem hiding this comment.
We already handle API rate limits in TIDAL, rate limiting should not be a concern here 🤔
Regardless, using limit as a workaround feels like a hack. If incremental updates are truly necessary, we should implement this as proper pagination. Otherwise, using a fixed limit means that, as long as my library doesn’t change, we’ll always update the same n albums. Here this could result in some albums/items never being updated or am I missing something crucial?
There was a problem hiding this comment.
The full command can exclude the recently updated ones, e.g., beet spotifysync -f 'spotify_updated:..-30d' ^genres:"Bollywood Instrumental" ^genres:"Bollywood Unwind" year- '<3000'
I use this daily to keep my popularity information updated while still leaving some daily API calls for imports and other operations.
There was a problem hiding this comment.
I still don’t see why this specifically needs a --limit parameter, feels pretty arbitrary.
If we can trust the spotify docs, rate limits apply at the app level, so if you hit them, everyone using that app (i.e., every beets user) will do too. If this is a real badly documented limit on the ip address level, adding a “max daily requests” option to the spotifysync command/spotify plugin seems like a better, more explicit solution than repurposing --limit as a workaround.
| ) | ||
| # Get items from arguments | ||
| items = lib.items(args) | ||
| items = lib.items(args, limit=opts.limit) |
There was a problem hiding this comment.
Would skip here too as the plugin is deprecated.
| opts.force_refetch or self.config["force"].get(bool), | ||
| ) | ||
|
|
||
| cmd.parser.add_limit_option() |
| quiet = self.config["quiet"].get(bool) or opts.quiet | ||
| self.calculate_bpm( | ||
| list(lib.items(args)), | ||
| list(lib.items(args, limit=opts.limit)), |
There was a problem hiding this comment.
I think a limit would make sense here if we had an offset parameter too for running the bpm update incrementally. I would not add limit for now here tho.
Instead of guessing how the |
I genuinely think we should avoid adding features/options unless we’ve identified at least one concrete use case for them. The fact that the If we have a use case for |
|
I think accepting Adding it only to selected query-based commands would make the CLI inconsistent and require users to remember which commands support the cap. The maintenance cost should remain small because parsing and validation are centralized, and the library query API already supports the limit. My preference is therefore to add |
|
There’s a big difference between using Before this change, limiting behavior lived in an opt‑in plugin, so users who wanted that footgun explicitly enabled it. Now it becomes a built‑in flag on core commands, which changes the risk profile. If you all agree that you really want/need this everywhere, I guess I’m overruled, it still feels like a strange choice to me. |
I also agree, because that would be the true replacement for the current limit plugin. Otherwise, the new implementation only partially replaces the limit function.
Users already know this if/when they use the |
|
Explicit doesn't necessarily mean safe. You're right that |
I would say yes, if that is what the user explicitly wants. Honestly, how is this any different than, say, |
|
The key difference is that Edit: To quote the
|
|
This is exactly why I have |
|
I understand that it works in this specific case. But the issue is that it’s a workaround for a deeper-seated issue. My point is that limits on mutations are generally considered a footgun in SQL systems, and the fact that you need or want a limit here itself points to broader issues (see my opinion on this here) |
|
I agree that predictable ordering matters when a limit is used with a mutating command. However, we also want to preserve the existing For that reason, I still think The SQLite |
ac43b8d to
7b2127f
Compare
|
I don't think the SQLite comparison applies here. Beets does not execute We also intend to preserve the existing I agree that |
Yeah that is what I meant by my remark earlier ("... updating one item at a time ..."). If a user applies a limit without an explicit sort, they need to know what the default sort is. That default must be consistent and always the same, so that
From a UX perspective, one could argue that a less obvious syntax here is actually preferable: most (if not all) users shouldn’t be using limits on mutations at all. Allowing it through the general query syntax ( Again, if you’re really convinced this is the right direction, don’t let my review block you. I just wanted to offer my perspective here. |
|
I do want to note that beets does not use arbitrary ordering. |
|
I do need an approval here either way 😅 |
Added a shared
CommonOptionsParser.add_limit_option()inbeets.uisolimitparsing and validation now live in one place instead of being reimplemented per command.Wired
limit=opts.limitthrough core query-based commands and many plugins by passing it directly intolib.items()andlib.albums(). This makes--limita consistent cross-command capability, not something mostly tied tolist.Kept the CLI shape compatible by using
--limitonly on commands where-lwas already taken for another meaning, avoiding flag conflicts without adding new command-specific behavior.Updated docs and changelog to reflect the broader architecture change: query-based commands now support built-in limiting, and the old
limitplugin guidance is reframed around that built-in behavior.Added parser and integration-oriented test coverage, plus a small
ipfsfollow-up fix, to make sure the new shared option is passed through safely and behaves consistently across commands.High-level impact
--limit, which makes them easier to preview and use incrementally.