Skip to content
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

Capitalize letters after punctuations #3411

Open
wants to merge 5 commits into
base: master
from

Conversation

@ColeBennett
Copy link

ColeBennett commented Oct 21, 2019

Addresses the issues noted in #3298

Revamps the current %title functionality, moving from Python's string.capwords() to using the python-titlecase package. Supports the original functionality while also capitalizing the first letter of words that follow a punctuation (such as ', ", /, etc).

@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch from a8f5638 to 10b4a50 Oct 21, 2019
=
@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch from 10b4a50 to 3acfd41 Oct 21, 2019
Copy link
Member

sampsyo left a comment

Hello! Thank you for looking into this. This code seems to be taken from titlecase.py. If we're going to go this route, I think it would be better to use a dependency on that module instead of duplicating the code in ours.

@@ -1482,10 +1481,91 @@ def tmpl_upper(s):
"""Covert a string to upper case."""
return s.upper()

# Regular expressions used by tmpl_title
SMALL = 'a|an|and|as|at|but|by|en|for|if|in|of|on|or|the|to|v\\.?|via|vs\\.?' # noqa: E501

This comment has been minimized.

Copy link
@sampsyo

sampsyo Oct 21, 2019

Member

Instead of #noqaing this line, it could just be broken up onto two lines:

SMALL = 'some stuff here' \
  'more stuff here'

This comment has been minimized.

Copy link
@ColeBennett

ColeBennett Oct 21, 2019

Author

Hi @sampsyo. That's correct, it was taken from that repository and has been modified. I considered adding a dependency, however I found that that it is not as well maintained and has some unnessesary code. Do you think it would be worth adding in this custom version for beets?

=
@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch from 435040c to 7b83731 Oct 22, 2019
@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Oct 22, 2019

I really do think it is best to keep the code in a library, where it has a shot at being maintained independently. 😃 Care to elaborate on what you mean by "not as well maintained"? Maybe we can help maintain it!

@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch 2 times, most recently from d3951e3 to 65f5c4b Oct 23, 2019
@ColeBennett

This comment has been minimized.

Copy link
Author

ColeBennett commented Oct 23, 2019

@sampsyo I've added the titlecase package to this PR. I actually agree with you now for this case, it's best to have that managed in an external package. It looks stable in its current state and the test cases I've added verify the functionality we need.

Thanks for taking a look at this! Let me know if there's anything else needed to get this merged in.

@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch from 65f5c4b to 2c39bd7 Oct 23, 2019
@ColeBennett ColeBennett force-pushed the austinmm:capitalize_letters_after_punctuations branch from 2c39bd7 to c196eca Oct 23, 2019
=
@nopoz

This comment has been minimized.

Copy link

nopoz commented Feb 15, 2020

Is it possible to get this pull reviewed and merged? Would love to get the title case working properly.

@sampsyo

This comment has been minimized.

Copy link
Member

sampsyo commented Feb 15, 2020

Thanks for the ping! It looks like I dropped the thread.

There are some unrelated changes here that would be great to roll back so we can get a clean merge… I'll make some comments now.

@@ -130,17 +130,17 @@ def format(self, value):
return time.strftime(beets.config['time_format'].as_str(),
time.localtime(value or 0))

def parse(self, string):
def parse(self, s):

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 15, 2020

Member

Let's change these argument names back (because the change is not relevant to the feature at hand).

self._setf(u'%title{$title}')
self._assert_dest(b'/base/The Title')
self._setf(u'%title{title}')
self._assert_dest(b'/base/Title')

This comment has been minimized.

Copy link
@sampsyo

sampsyo Feb 15, 2020

Member

I'm not sure why this test changed? We should add a new one if we just want to test non-variable title-casing.

@nopoz

This comment has been minimized.

Copy link

nopoz commented Mar 19, 2020

@ColeBennett can you make these requested changes? I'm itching to get this merged into the main repo. 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.