Fix titleize so already capitalized words are not dropped #4525

Merged
merged 1 commit into from Feb 16, 2016

Conversation

Projects
None yet
4 participants
@atomicules
Contributor

atomicules commented Feb 15, 2016

Previously titleize used capitalize! which has the side effect of
returning nil for anything already starting with a capital letter. This
commit changes it to just capitalize.

Example, before:

A file "2016-01-01-This-is-a-title-with-Capitals.markdown" would return "Is A
Title With" for post.title

Example, after:

A file "2016-01-01-This-is-a-title-with-Capitals.markdown" will return "This Is A
Title With Capitals" for post.title

Note: this is based off tag v3.1.1 as at the time of cloning this master
was causing me problems with markdown parsing on one of my posts and I
only ended up looking into this because I was trying to write a blog
post after updating Jekyll to 3.1.1.

Not sure if tests are in the right place. Seems to be correct place as
have added tests for differently capitalised filenames with and without
dates.

Fix problem introduced in 67f8425

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 15, 2016

Contributor

This is a complicated fix to a simple problem. There is no need for an entire scenario, a test case inside of tests sure, and without the extra fixtures as you only need to test to ensure that the method returns all the proper words.

Contributor

envygeeks commented Feb 15, 2016

This is a complicated fix to a simple problem. There is no need for an entire scenario, a test case inside of tests sure, and without the extra fixtures as you only need to test to ensure that the method returns all the proper words.

@atomicules

This comment has been minimized.

Show comment
Hide comment
@atomicules

atomicules Feb 15, 2016

Contributor

Thanks for taking a look.

On 15-Feb-2016 08:02:59, Jordon Bedwell wrote:

This is a complicated fix to a simple problem.

Well, the fix is simple, but the tests I'll give you :)

There is no need for an entire scenario, a test case inside of tests
sure, and without the extra fixtures as you only need to test to
ensure that the method returns all the proper words.

If you could give me guidance as to where I should fit the tests in
that would speed up me being able to implement it, but I'll start taking a
look anyway to simplify them.

My reason for wanting to test filenames with and without dates is
because I wasn't sure to start with if titleize was using
capitalize! intentionally to remove the date from the start of the
filename.

Contributor

atomicules commented Feb 15, 2016

Thanks for taking a look.

On 15-Feb-2016 08:02:59, Jordon Bedwell wrote:

This is a complicated fix to a simple problem.

Well, the fix is simple, but the tests I'll give you :)

There is no need for an entire scenario, a test case inside of tests
sure, and without the extra fixtures as you only need to test to
ensure that the method returns all the proper words.

If you could give me guidance as to where I should fit the tests in
that would speed up me being able to implement it, but I'll start taking a
look anyway to simplify them.

My reason for wanting to test filenames with and without dates is
because I wasn't sure to start with if titleize was using
capitalize! intentionally to remove the date from the start of the
filename.

@atomicules

This comment has been minimized.

Show comment
Hide comment
@atomicules

atomicules Feb 15, 2016

Contributor
Contributor

atomicules commented Feb 15, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 15, 2016

Contributor

test_utils.rb is the perfect place for it! And yes, the fix is super simple and thanks for catching it. I didn't even think about that when we switched to #capitalize!

Contributor

envygeeks commented Feb 15, 2016

test_utils.rb is the perfect place for it! And yes, the fix is super simple and thanks for catching it. I didn't even think about that when we switched to #capitalize!

@atomicules

This comment has been minimized.

Show comment
Hide comment
@atomicules

atomicules Feb 15, 2016

Contributor
Contributor

atomicules commented Feb 15, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 15, 2016

Contributor

Yes please!

Contributor

envygeeks commented Feb 15, 2016

Yes please!

Fix titleize_slug so already capitalized words are not dropped
Previously `titleize` used `capitalize!` which has the side effect of
returning `nil` for anything already starting with a capital letter. This
commit changes it to just `capitalize`.

Example, before:

A file "2016-01-01-This-is-a-title-with-Capitals.markdown" would return "Is A
Title With" for `post.title`

Example, after:

A file "2016-01-01-This-is-a-title-with-Capitals.markdown" will return "This Is A
Title With Capitals" for `post.title`

Tests added for `titleize_slug` in test_utils.rb

Fix problem introduced in 67f8425

References #4525
@atomicules

This comment has been minimized.

Show comment
Hide comment
@atomicules

atomicules Feb 15, 2016

Contributor
Contributor

atomicules commented Feb 15, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 16, 2016

Member

This looks great to me. My gratitude!

@jekyllbot: merge +bug

Member

parkr commented Feb 16, 2016

This looks great to me. My gratitude!

@jekyllbot: merge +bug

jekyllbot added a commit that referenced this pull request Feb 16, 2016

@jekyllbot jekyllbot merged commit db9865e into jekyll:master Feb 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@parkr parkr added this to the 3.1.2 milestone Feb 16, 2016

jekyllbot added a commit that referenced this pull request Feb 16, 2016

@parkr parkr added the bug label Feb 16, 2016

@atomicules atomicules deleted the atomicules:capitalize-in-titleize branch Feb 16, 2016

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.