Improved permalinks for pages and collections #3538

Merged
merged 1 commit into from Mar 4, 2015

Conversation

Projects
None yet
4 participants
@willnorris
Contributor

willnorris commented Mar 3, 2015

This updates the default permalink style for pages and collections to match the site-wide 'permalink' setting (more detailed notes in the commit message).

I'm not wild about the method name Utils.append_permalink_ending, but at least it's accurate. I'm certainly open to suggestions for a better name there.

I also have not yet written the user-facing docs describing how this works. I'll certainly do that as part of this PR, but wanted to go ahead and get some 馃憖 on the implementation before then. I also wasn't quite sure how we handle docs for features that aren't yet in the current stable release. Do we just hold off pushing those live? Or do we just say that the docs on jekyllrb.com correspond to the latest master branch, not necessarily any particular released version?

@parkr

View changes

test/test_utils.rb
@@ -166,4 +166,18 @@ class TestUtils < JekyllUnitTest
end
end
+ context "The \`Utils.append_permalink_ending\` method" do
+ should "handle built-in permalink styles" do
+ assert_equal "/:basename/", Utils.append_permalink_ending("/:basename", :pretty)

This comment has been minimized.

@parkr

parkr Mar 4, 2015

Member

What if I have "/:basename/", :pretty?

@parkr

parkr Mar 4, 2015

Member

What if I have "/:basename/", :pretty?

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

you wouldn't. The value of template is not user-supplied, it's hardcoded in the method calls in document.rb and page.rb so we always control what the value is, and the docs for the method explicitly say that template should not have a trailing slash or extension.

@willnorris

willnorris Mar 4, 2015

Contributor

you wouldn't. The value of template is not user-supplied, it's hardcoded in the method calls in document.rb and page.rb so we always control what the value is, and the docs for the method explicitly say that template should not have a trailing slash or extension.

This comment has been minimized.

@envygeeks

envygeeks Mar 4, 2015

Contributor

馃憥 to hardcoding anything that can be user configurable.

@envygeeks

envygeeks Mar 4, 2015

Contributor

馃憥 to hardcoding anything that can be user configurable.

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

huh? This particular value isn't user configurable... it's the default template used for page/collection permalinks. Look at the code it is replacing... it's already hardcoded. Now we're just making the suffix match what the user has in their permalink setting in _config.yaml. Users can always provide their own per-page permalink value in the front matter, or the per-collection permalink setting in _config.yaml

@willnorris

willnorris Mar 4, 2015

Contributor

huh? This particular value isn't user configurable... it's the default template used for page/collection permalinks. Look at the code it is replacing... it's already hardcoded. Now we're just making the suffix match what the user has in their permalink setting in _config.yaml. Users can always provide their own per-page permalink value in the front matter, or the per-collection permalink setting in _config.yaml

@parkr

View changes

lib/jekyll/utils.rb
+ def append_permalink_ending(template, permalink_style)
+ case permalink_style
+ when :pretty
+ template << "/"

This comment has been minimized.

@parkr

parkr Mar 4, 2015

Member

Dealing with user input here, it's possible this could result in /:basename//.

@parkr

parkr Mar 4, 2015

Member

Dealing with user input here, it's possible this could result in /:basename//.

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

template isn't user input

@willnorris

willnorris Mar 4, 2015

Contributor

template isn't user input

@parkr

View changes

lib/jekyll/utils.rb
+ # # => "/:basename"
+ #
+ # Returns the updated permalink template
+ def append_permalink_ending(template, permalink_style)

This comment has been minimized.

@parkr

parkr Mar 4, 2015

Member

Yeah, this is a bit fuzzy when just skimming the code. "Ending" is kind of vague and we could shorten append to add? Maybe add_permalink_suffix? Not quite sure what would be best here.

@parkr

parkr Mar 4, 2015

Member

Yeah, this is a bit fuzzy when just skimming the code. "Ending" is kind of vague and we could shorten append to add? Maybe add_permalink_suffix? Not quite sure what would be best here.

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

add_permalink_suffix sounds good to me. done.

@willnorris

willnorris Mar 4, 2015

Contributor

add_permalink_suffix sounds good to me. done.

+ when :pretty
+ template << "/"
+ when :date, :ordinal, :none
+ template << ":output_ext"

This comment has been minimized.

@parkr

parkr Mar 4, 2015

Member

Same here, is it possible this could end up being /:basename:output_ext:output_ext?

@parkr

parkr Mar 4, 2015

Member

Same here, is it possible this could end up being /:basename:output_ext:output_ext?

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

same as above, this shouldn't ever happen. We could always add a check here to ensure that it doesn't, but that seems unnecessary to me.

@willnorris

willnorris Mar 4, 2015

Contributor

same as above, this shouldn't ever happen. We could always add a check here to ensure that it doesn't, but that seems unnecessary to me.

@parkr

View changes

lib/jekyll/collection.rb
@@ -170,7 +170,8 @@ def write?
#
# Returns the URL template to render collection's documents at.
def url_template
- metadata.fetch('permalink', "/:collection/:path:output_ext")
+ default = Utils.append_permalink_ending("/:collection/:path", site.permalink_style)
+ metadata.fetch('permalink', default)

This comment has been minimized.

@parkr

parkr Mar 4, 2015

Member

Maybe the default should be lazy-loaded via a block?

@parkr

parkr Mar 4, 2015

Member

Maybe the default should be lazy-loaded via a block?

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

and now I've reached the limit of my ruby knowledge :) I'm not sure I know what that would look like.

@willnorris

willnorris Mar 4, 2015

Contributor

and now I've reached the limit of my ruby knowledge :) I'm not sure I know what that would look like.

This comment has been minimized.

@envygeeks

envygeeks Mar 4, 2015

Contributor

@willnorris: http://ruby-doc.org//core-2.2.0/Hash.html#method-i-fetch

metadata.fetch('permalink') do
  Utils.append_permalink_ending("/:collection/:path", site.permalink_style)
end
@envygeeks

envygeeks Mar 4, 2015

Contributor

@willnorris: http://ruby-doc.org//core-2.2.0/Hash.html#method-i-fetch

metadata.fetch('permalink') do
  Utils.append_permalink_ending("/:collection/:path", site.permalink_style)
end

This comment has been minimized.

@willnorris

willnorris Mar 4, 2015

Contributor

ah, thanks. fixed.

@willnorris

willnorris Mar 4, 2015

Contributor

ah, thanks. fixed.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 4, 2015

Member

Woot! Looking pretty good, just a couple questions above.

Member

parkr commented Mar 4, 2015

Woot! Looking pretty good, just a couple questions above.

@parkr parkr added the Enhancement label Mar 4, 2015

@parkr parkr modified the milestone: 3.0 Mar 4, 2015

Improved permalinks for pages and collections
This updates the default permalink style for pages and collections to
match the site-wide 'permalink' setting.  If the permalink setting
contains a trailing slash, either explicitly or by being set to
':pretty', then pages and collections permalinks will contain trailing
slashes by default as well.  Similarly, if the permalink setting
contains a trailing ':output_ext', so will pages and collections.  If
the permalink setting contains neither a trailing slash or extension,
neither will pages or collections.

This impacts only the default permalink structure for pages and
collections.  Permalinks set in the frontmatter of an individual page
take precedence, as does the permalink setting for a specific
collection.

Fixes #2691
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 4, 2015

Member

I'm satisfied with this. Let's get it into the next beta so it can be thoroughly tested before we release it. :shipit:

We'll also want to keep an eye on how it affects performance, if at all.

Member

parkr commented Mar 4, 2015

I'm satisfied with this. Let's get it into the next beta so it can be thoroughly tested before we release it. :shipit:

We'll also want to keep an eye on how it affects performance, if at all.

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor

@parkr: what about user-facing docs (e.g. http://jekyllrb.com/docs/permalinks/)? How do we handle that for features like this that are not in a stable release yet?

Contributor

willnorris commented Mar 4, 2015

@parkr: what about user-facing docs (e.g. http://jekyllrb.com/docs/permalinks/)? How do we handle that for features like this that are not in a stable release yet?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 4, 2015

Contributor

@parkr @willnorris Do we not have edge docs?

Contributor

envygeeks commented Mar 4, 2015

@parkr @willnorris Do we not have edge docs?

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor

@envygeeks we very well might, I'm just not completely up to speed on how all of that is setup. I can write the docs as part of this PR, but just want to make sure they don't accidentally get published somewhere they shouldn't.

Contributor

willnorris commented Mar 4, 2015

@envygeeks we very well might, I'm just not completely up to speed on how all of that is setup. I can write the docs as part of this PR, but just want to make sure they don't accidentally get published somewhere they shouldn't.

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor

I'm satisfied with this. Let's get it into the next beta so it can be thoroughly tested before we release it. :shipit:

I'm going to go ahead and check this in so we can start testing it. I'll do the user docs in a separate PR, and can figure out then what to do about edge docs.

Contributor

willnorris commented Mar 4, 2015

I'm satisfied with this. Let's get it into the next beta so it can be thoroughly tested before we release it. :shipit:

I'm going to go ahead and check this in so we can start testing it. I'll do the user docs in a separate PR, and can figure out then what to do about edge docs.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 4, 2015

Member

We don't have edge docs. I like tip.golang.org, but it's hard to keep fixes and changes due to upgrading features separate. We'd have to argue that once we release something, the docs are as-is and you just go to like jekyllrb.com/2.5.3/ and it's the docs as they were at the v2.5.3 tag.

We usually add a note, check out the bottom of this: http://jekyllrb.com/docs/home/

Member

parkr commented Mar 4, 2015

We don't have edge docs. I like tip.golang.org, but it's hard to keep fixes and changes due to upgrading features separate. We'd have to argue that once we release something, the docs are as-is and you just go to like jekyllrb.com/2.5.3/ and it's the docs as they were at the v2.5.3 tag.

We usually add a note, check out the bottom of this: http://jekyllrb.com/docs/home/

willnorris added a commit that referenced this pull request Mar 4, 2015

Merge pull request #3538 from willnorris/permalink
Improved permalinks for pages and collections

@willnorris willnorris merged commit f6f2626 into jekyll:master Mar 4, 2015

1 check passed

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

willnorris added a commit that referenced this pull request Mar 4, 2015

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Mar 4, 2015

Contributor

ah, started merging this pr just before seeing your comment :). I'll add an unreleased note for the relevant changes to the permalink docs.

Contributor

willnorris commented Mar 4, 2015

ah, started merging this pr just before seeing your comment :). I'll add an unreleased note for the relevant changes to the permalink docs.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 5, 2015

Member

Thank you! Please ref this pr in that commit.

Member

parkr commented Mar 5, 2015

Thank you! Please ref this pr in that commit.

@willnorris willnorris deleted the willnorris:permalink branch Mar 5, 2015

@willnorris willnorris referenced this pull request Mar 9, 2015

Merged

update permalinks docs #3556

kevinoid added a commit to kevinoid/kevinlocke.name that referenced this pull request Jul 23, 2016

Set page permalink style to "none"
Due to the changes in jekyll/jekyll@90865d (Jekyll 3.1.0+) .xhtml pages
are not treated like .html pages which are placed in subdirectories
based on the sitewide permalink setting (from jekyll/jekyll#3538).

To avoid this, set the sitewide permalink setting to "none" and set a
permalink default value for drafts and pages with the desired permalink
for those types.

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

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