PICARD-1386: Add $title function #1018
Conversation
phw
changed the title
picard/script.py
Outdated
| @@ -846,6 +846,17 @@ def func_ne_any(parser, x, *args): | |||
| """ | |||
| return func_not(parser, func_eq_all(parser, x, *args)) | |||
|
|
|||
| def func_title(parser, text): | |||
| """ Title-case a text """ | |||
There was a problem hiding this comment.
Please make a more complete description of the function, including an example.
Also add a test for it in https://github.com/metabrainz/picard/blob/master/test/test_script.py
picard/script.py
Outdated
| words.remove(words[0]) | ||
| for i in words: | ||
| capital.append(i.capitalize()) | ||
| return " ".join(capital) |
There was a problem hiding this comment.
How does this differ from string.capwords ?
There was a problem hiding this comment.
@virusMac Thanks a lot, this is already looking pretty good. I actually like your general approach of implementing this function (see my comment below). But nevertheless one change, please use the implementation at https://github.com/metabrainz/picard-plugins/blob/2.0/plugins/titlecase/titlecase.py as a basis for this function. This is a bit advanced and handles cases with word boundaries other than blanks like the examples below.
| s | title(s) |
|---|---|
| unchained (tales of the unexcpected) | Unchained (Tales Of The Unexcpected) |
| …alone | …Alone |
For completeness, Python already provides a title function for strings, you can just do text.title(). But this for example already would fail on your #1abc 4def - g test case and convert it to #1Abc 4Def - G, which is definitely unexpected.
Now just one note about your own implementation of the function. As I said I like the general approach you took, but you could have simplified it by not treating the first word special like so:
def func_title(parser, text):
if not text:
return ""
words = text.split()
capital = []
for i in words:
capital.append(i.capitalize())
return " ".join(capital)
That avoids the need to remove the first word again for the list :)
test/test_script.py
Outdated
| self.assertScriptResultEquals("$title(Abc Def G)","Abc Def G") | ||
| self.assertScriptResultEquals("$title(abc def g)","Abc Def G") | ||
| self.assertScriptResultEquals("$title(#1abc 4def - g)","#1abc 4def - G") | ||
|
|
There was a problem hiding this comment.
Oh, please keep the test cases and add also the two test cases I mentioned in my previous comment. I think then we are good to go here :)
phw
changed the title
|
Please rebase and squash this before merging. |
|
Also make sure your commit message follows the contribution guidelines |
virusMac
force-pushed
the
master
branch
2 times, most recently
from
ca9e90f
to
15419cd
Compare
|
Omg, i've accidentally done something wrong... |
|
|
virusMac
force-pushed
the
master
branch
5 times, most recently
from
0536206
to
ca9e90f
Compare
+ unit tests for func_test Add func_title and tests for it in test_script.py added 2 tests to func_title added 2 tests to func_title
|
@phw, the commits were still not fixed? |
|
@samj1912 There was some mess with unrelated commits in the PR in between, but it had been fixed. |
Summary
Problem
Creating more flexible title-case function, than function in plugin titlecase
Solution
Added func_title function
Action