-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ftintitle plugin now allows a custom format to be defined (Correct Branch) #1377
Conversation
Hi, Finally shouldn't we allow multiple formats: a list of formats instead of a single format? |
I'll add the tests. How would you go about testing a I didn't add the default value because not all plugins register their defaults in the default config (ftintitle wasn't there previously, do I didn't add to it), I will add it however if required. In regards to into the multiple formats, why would you want to define a list. I would want my music to follow the same format of how I define my feat. I'm unsure as to the use case in which this would be relevant. |
Why would you want to test it? What's important is testing the
Good point.
|
Hi @brunal I have simplified the implementation, as the previous code was passing parameters into functions that it didn't need to. Thank you also for pointing me in the direction for the functional tests. I learnt a great deal about them and gave it my best shot. The ftintitle plugin has no functional tests at all, so I created a new class for them, and have added a few, not only surrounding my changes but also for the "drop" feat option too. I hope this is okay. |
Looks like I have some issues to sort out in the build. I'll get on it. |
Build looks good. Comments are welcome :) |
@@ -24,6 +24,9 @@ file. The available options are: | |||
- **drop**: Remove featured artists entirely instead of adding them to the | |||
title field. | |||
Default: ``no``. | |||
- **format**: Defines the format for the feat part of the new title field. | |||
In this format the ``{0}`` is used to define where the featured artists are placed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a period at the end of the sentence.
LGTM! I made a few very tiny style suggestions, after which I think this will be all ready. And the tests are awesome! |
Hi @sampsyo :) |
ftintitle plugin now allows a custom format to be defined (Correct Branch)
Awesome! ✨ Thanks for implementing this. All merged. |
(Apologies for the duplicate request, but I referenced the incorrect branch. Thanks.)
I have updated the ftintitle plugin to allow a custom format to be defined. The default format is that which was previously hardcoded.
This pull request allows formats like the list below to be defined:
This is my first commit to an open source project, so I'm still working my way around the beets project. Any feedback would be great.
Thanks