Fix autobpm and improve its docs #5389
Conversation
snejus
force-pushed
the
autobpm-fix-and-improve-docs
branch
from
238ba9c to
16736db
Compare
| exc, | ||
| ) | ||
|
|
||
| kwargs = self.config["beat_track_kwargs"].flatten() |
There was a problem hiding this comment.
Just checking, there's no security considerations here, right? cc @Serene-Arc
| "autobpm", help="detect and add bpm from audio using Librosa" | ||
| ) | ||
| cmd.func = self.command | ||
| return [cmd] | ||
|
|
||
| def command(self, lib, opts, args): | ||
| self.calculate_bpm(lib.items(ui.decargs(args)), write=ui.should_write()) | ||
| def command(self, lib: Library, _, args: list[str]) -> None: |
There was a problem hiding this comment.
I agree that the unused arguments should be marked, but I think their names should be preserved, i.e. using _opts instead of just _. It's just a bit clearer, especially since the whole codebase isn't well-typed yet. Also, a type annotation should still be used for the argument.
There was a problem hiding this comment.
I agree regarding general clarity, but do you reckon this is needed here given that this argument is unused? I find that _ in such case clearly communicates that. I find the presence of the type a bit confusing if the argument is not used.
Note that my opinion here is mostly based on refactoring interfaces like the one that BeetsPlugin provides: in this case using _ notation allows the interface (and the type) to change without the need to update the code here.
Essentially, I'm applying the ISP principle to the function arguments here
Within object-oriented design, interfaces provide layers of abstraction that simplify code and create a barrier preventing coupling to dependencies. A system may become so coupled at multiple levels that it is no longer possible to make a change in one place without necessitating many additional changes.[1] Using an interface or an abstract class can prevent this side effect.
There was a problem hiding this comment.
That makes sense, I'm just not used to it. Any preference, @Serene-Arc?
snejus
force-pushed
the
autobpm-fix-and-improve-docs
branch
3 times, most recently
from
2a7f33d to
ca5dd1f
Compare
|
@JOJ0 I noticed your comment under #4923 regarding difficulties calculating BPM for offbeaty tracks: I've been having the same issue - I found that adjusting autobpm:
beat_track_kwargs:
start_bpm: 160Just giving a shout making sure you're aware! |
Given that librosa has been introducing breaking changes like there's no tomorrow, use '^' version specifier to only allow updating the patch version.
Additionally, fix DefaultTemplateFunctions._func_names definition.
snejus
force-pushed
the
autobpm-fix-and-improve-docs
branch
from
ca5dd1f to
111686e
Compare
|
@Serene-Arc @bal-e do we think this could be merged? |
|
thanks for doing this! |
|
Also from my end many thanks @snejus and thanks for the tip with configuring bpm min value. will try that! |
Description
Fixes #5289 and #5185
Tried using the
autobpmplugin and found a couple of issues:librosadependency was missing inpyproject.tomlbeettake over 4 seconds to start up.resampybeat_trackfunction returned unexpected type which made the plugin and the entire import process fail.Addressed each of the above, slightly refactored the plugin and added tests.
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)