New Plugin: Titlecase#6133
Conversation
… for pre or post import
|
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. |
semohr
left a comment
There was a problem hiding this comment.
The documentation is lovely! Added some small comments but looks good overall.
|
Want to add an option intercept candidates for auto tagging before the distances have been calculated, so the changes the plugin makes are more obvious on import - have reverted to draft until then. |
|
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6133 +/- ##
==========================================
+ Coverage 67.49% 67.71% +0.22%
==========================================
Files 136 137 +1
Lines 18533 18652 +119
Branches 3126 3148 +22
==========================================
+ Hits 12508 12630 +122
+ Misses 5361 5359 -2
+ Partials 664 663 -1
🚀 New features to boost your workflow:
|
c142944 to
dcac9ba
Compare
There was a problem hiding this comment.
This looks great to me! I added some minor comment.
We might want to cleanup the git commit history a bit. E.g. there are quite a few merges from master which seem unnecessary to me. We could also do a squash, depends a bit which commit information we want to keep.
Further the poetry.lock file changed a bit more than I expected for a singular addition of the titlecase dependency. We might want to not update the other dependencies here.
Changelog entry is missing.
I think we could squash and clean this up to whatever degree feels good, I'll defer to your opinion on it since you have more experience with this project.
I'll double check the pyproject.toml and lock file to make sure they're not too different from the master branch, I think we might be seeing the merge in of the recent updates? It should've run with --no-update.... |
I just used the github diff view, it is a bit flakey recently. Should be gone after a rebase than 👍 |
|
Think we should be good to go now - probably easier just to squash merge this, especially since it's just one whole new feature. I'll make sure to squash irrelevant commits as I go next time. |
There was a problem hiding this comment.
I just had one last look!
There is one type:ignore that is not required anymore, otherwise I'm very happy to approve this.
If you are up for, could you add a test for the imported function too:
beetsplug/titlecase.py 120 8 44 3 92.07% 81->exit, 83, 161->157, 231-237
I don't think we will ever write one if we don't do in this PR ;)
|
Got that test added. It is isn't as much of an integration test with the whole import flow, but it makes sure that the logic of the function does what it is supposed to. |
This plugin aims to address the shortcomings of the %title function, as brought up in issues #152, #3298 and an initial look to improvement with #3411. It supplies a new string format command,
%titlecasewhich doesn't interfere with any prior expected behavior of the%titleformat command.It also adds the ability to apply titlecase logic to metadata fields that a user selects, which is useful if you, like me, are looking for stylistic consistency and the minor stylistic differences between Musizbrainz, Discogs, Deezer etc, with title case are slightly infuriating.
This will add an optional dependency of titlecase, which allows the titlecase core logic to be externally maintained.
If there's not enough draw to have this as a core plugin, I can also spin this into an independent one, but it seemed like a recurring theme that the %title string format didn't really behave as expected, and I wanted my metadata to match too.
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)