Use category in downcase only for URL #2571

Merged
merged 2 commits into from Jan 17, 2015

Conversation

Projects
None yet
4 participants
@yous
Contributor

yous commented Jul 5, 2014

Resolves #1739.
It preserves letter case of category, but return category.downcase for url_placeholders. Also added some proper tests. If there are needed updates for documentation, please let me know.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 7, 2014

Contributor

I'm 👍 on this.

Contributor

envygeeks commented Jul 7, 2014

I'm 👍 on this.

lib/jekyll/post.rb
@@ -223,7 +223,7 @@ def url_placeholders
:title => slug,
:i_day => date.strftime("%d").to_i.to_s,
:i_month => date.strftime("%m").to_i.to_s,
- :categories => (categories || []).map { |c| c.to_s }.join('/'),
+ :categories => (categories || []).map { |c| c.to_s.downcase }.join('/'),

This comment has been minimized.

@parkr

parkr Jul 7, 2014

Member

This will have to be uniq'd as well. If I have Movies and movies, I don't want /movies/movies/2014/07/06/my-post.html.

@parkr

parkr Jul 7, 2014

Member

This will have to be uniq'd as well. If I have Movies and movies, I don't want /movies/movies/2014/07/06/my-post.html.

@yous

This comment has been minimized.

Show comment
Hide comment
@yous

yous Jul 7, 2014

Contributor

Now it removes duplicated categories like MixedCase and Mixedcase only for URL, and added a test for it.

Contributor

yous commented Jul 7, 2014

Now it removes duplicated categories like MixedCase and Mixedcase only for URL, and added a test for it.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 10, 2014

Member

I have to think about this more. This is a bit convoluted – perhaps we can create a Category struct-based class for each category and have a name and slug value for each? Then you can weed out based on the slugs and keep all this data straight.

Member

parkr commented Jul 10, 2014

I have to think about this more. This is a bit convoluted – perhaps we can create a Category struct-based class for each category and have a name and slug value for each? Then you can weed out based on the slugs and keep all this data straight.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 10, 2014

Contributor

I don't think this leaves out that option for later. This immediately solves a problem that can be refined later with a better solution. Since this really adds no major debt at all to that idea to begin with, IMO it wouldn't hurt to just merge it to solve the problem and then flag the ticket as incomplete and make a note of what should happen in the future.

That's just my opinion on code that introduces no real debt but solves an immediate problem.

Contributor

envygeeks commented Jul 10, 2014

I don't think this leaves out that option for later. This immediately solves a problem that can be refined later with a better solution. Since this really adds no major debt at all to that idea to begin with, IMO it wouldn't hurt to just merge it to solve the problem and then flag the ticket as incomplete and make a note of what should happen in the future.

That's just my opinion on code that introduces no real debt but solves an immediate problem.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 31, 2014

Member

This needs a rebase/merge of master.

Member

parkr commented Jul 31, 2014

This needs a rebase/merge of master.

@yous

This comment has been minimized.

Show comment
Hide comment
@yous

yous Aug 1, 2014

Contributor

Rebased. 😄

Contributor

yous commented Aug 1, 2014

Rebased. 😄

@yous

This comment has been minimized.

Show comment
Hide comment
@yous

yous Aug 2, 2014

Contributor

Rebased again.

Contributor

yous commented Aug 2, 2014

Rebased again.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 6, 2014

Member

How does this affect backwards-compatibility?

Member

parkr commented Aug 6, 2014

How does this affect backwards-compatibility?

yous added some commits Jul 5, 2014

@yous

This comment has been minimized.

Show comment
Hide comment
@yous

yous Aug 7, 2014

Contributor

Rebased again.

I don't think this will rename directories and files. Also category urls aren't affected because they will be always down-cased. Can you tell me any other issues that I should consider?

Contributor

yous commented Aug 7, 2014

Rebased again.

I don't think this will rename directories and files. Also category urls aren't affected because they will be always down-cased. Can you tell me any other issues that I should consider?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 7, 2014

Member

My thought is if one is manipulating categories in Liquid and providing a URL related to it (maybe you've created a category page where all your posts of a certain category are printed on a given page) or checking if a post has a given category. In the current model, any capitalization variation on the category is eliminated, making it quite easy. If we change this behaviour, we may find ourselves with a lot of broken sites. This almost feels like a 3.0 change because it has the potential to cause significant template changes. It's certainly possible to change the templates to conform, but this PR in its present form does not provide any backwards-compatibility or deprecation messages.

We want to be able to have capitalization in the printed representation of the category, but any logic around it (esp comparison) should be case-insensitive.

Member

parkr commented Aug 7, 2014

My thought is if one is manipulating categories in Liquid and providing a URL related to it (maybe you've created a category page where all your posts of a certain category are printed on a given page) or checking if a post has a given category. In the current model, any capitalization variation on the category is eliminated, making it quite easy. If we change this behaviour, we may find ourselves with a lot of broken sites. This almost feels like a 3.0 change because it has the potential to cause significant template changes. It's certainly possible to change the templates to conform, but this PR in its present form does not provide any backwards-compatibility or deprecation messages.

We want to be able to have capitalization in the printed representation of the category, but any logic around it (esp comparison) should be case-insensitive.

@yous

This comment has been minimized.

Show comment
Hide comment
@yous

yous Aug 7, 2014

Contributor

True. Then each category should be an instance of Category class, in next major version of Jekyll.

Contributor

yous commented Aug 7, 2014

True. Then each category should be an instance of Category class, in next major version of Jekyll.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 29, 2014

Member

Want to take a crack at the more direct/verbose option? We're working on Jekyll 3 now.

Member

parkr commented Dec 29, 2014

Want to take a crack at the more direct/verbose option? We're working on Jekyll 3 now.

@parkr parkr added the fix label Dec 29, 2014

@parkr parkr merged commit ba2e139 into jekyll:master Jan 17, 2015

1 check passed

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

@parkr parkr added this to the 3.0 milestone Jan 17, 2015

parkr added a commit that referenced this pull request Jan 17, 2015

@yous yous deleted the yous:patch-mixed-case-category branch Feb 4, 2015

@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.