ftintitle plugin now allows a custom format to be defined (Correct Branch)

[ab623]

(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:

• feat. ...
• (Feat. ...)
• ft. ...
• featuring ...

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

[brunal]

Hi,
Could you add a test in test/test_ftintitle.py? And set feat. as the default value in the default config (beets/default_config.yaml)?

Finally shouldn't we allow multiple formats: a list of formats instead of a single format?

[ab623]

I'll add the tests. How would you go about testing a item.store()

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.

[brunal]

I'll add the tests. How would you go about testing a item.store()

Why would you want to test it? What's important is testing the ft_in_title() method IMO. And if you want to do functional testing with TestHelper.run_command() (which is a nice additional test) you don't need to mock Item.store() since it happens in a temporary beets environment (see TestHelper.setup_beets() and its usages).
In general (but not in this case) you would mock the method: item.store = mock.Mock. That way you can even assert how many times it's been called and with what arguments (see usages of assert_has_calls and assert_called_once).

I didn't add the default value because not all plugins register their defaults in the default config [...]

Good point.

In regards to into the multiple formats, why would you want to define a list.
Never mind. I don't use that plugin and I was mistaken on its functionality.

[ab623]

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.

[ab623]

Looks like I have some issues to sort out in the build. I'll get on it.

[ab623]

Build looks good. Comments are welcome :)

[sampsyo]

Missing a period at the end of the sentence.

[sampsyo]

LGTM! I made a few very tiny style suggestions, after which I think this will be all ready.

And the tests are awesome!

[ab623]

Hi @sampsyo
The changes you requested have been added.

:)

[sampsyo]

Awesome! ✨ Thanks for implementing this. All merged.