Added per post excerpt_separator functionality #3274

Merged
merged 2 commits into from Jan 17, 2015

Conversation

Projects
None yet
5 participants
@majioa
Contributor

majioa commented Jan 9, 2015

Added per post excerpt_separator functionality, so you are able to specify :excerpt_separator (as well as just :excerpt) key direct inside the post YAML, to make an excerpt based on the value in the post. Tests
were also added.

Added per post excerpt_separator functionality, so you are able to
specify :excerpt_separator (as well as just :excerpt) key direct inside
the post YAML, to make an excerpt based on the value in the post. Tests
were also added.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 9, 2015

Member

Nice work. I'm curious though: what's the use case for this?

Member

parkr commented Jan 9, 2015

Nice work. I'm curious though: what's the use case for this?

@majioa

This comment has been minimized.

Show comment
Hide comment
@majioa

majioa Jan 9, 2015

Contributor

@parkr I have a lot of documents in which I use the default separator, and some other docs, in which I use the specific one, but for that cases I don't want to specify the excerpt explicitly, just want to reuse the heap of those documents.

Contributor

majioa commented Jan 9, 2015

@parkr I have a lot of documents in which I use the default separator, and some other docs, in which I use the specific one, but for that cases I don't want to specify the excerpt explicitly, just want to reuse the heap of those documents.

get procedure for default excerpt separator for both cases site and p…
…age was

moved to the post's specific method :excerpt_separator.
lib/jekyll/post.rb
+ #
+ # Returns the post excerpt_separator
+ def excerpt_separator
+ data.fetch('excerpt_separator') { "" }

This comment has been minimized.

@parkr

parkr Jan 10, 2015

Member

This should fall back on the site excerpt_separator and probably also on the front matter defaults.

@parkr

parkr Jan 10, 2015

Member

This should fall back on the site excerpt_separator and probably also on the front matter defaults.

This comment has been minimized.

@majioa

majioa Jan 10, 2015

Contributor

@parkr do you mean instead of returning "" it should return site excerpt_separator?

@majioa

majioa Jan 10, 2015

Contributor

@parkr do you mean instead of returning "" it should return site excerpt_separator?

This comment has been minimized.

@parkr

parkr Jan 10, 2015

Member

I mean instead of just returning "" straight away, it should check the defaults and the site first.

@parkr

parkr Jan 10, 2015

Member

I mean instead of just returning "" straight away, it should check the defaults and the site first.

This comment has been minimized.

@alfredxing

alfredxing Jan 10, 2015

Member

@parkr The fallback is above in excerpt.rb, though I agree that the fallback should be here instead of there.

@alfredxing

alfredxing Jan 10, 2015

Member

@parkr The fallback is above in excerpt.rb, though I agree that the fallback should be here instead of there.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2015

Member

Any thoughts, @jekyll/core?

Member

parkr commented Jan 10, 2015

Any thoughts, @jekyll/core?

lib/jekyll/excerpt.rb
- separator = site.config['excerpt_separator']
+ separator = !post.excerpt_separator.empty? &&
+ post.excerpt_separator ||
+ site.config['excerpt_separator']

This comment has been minimized.

@alfredxing

alfredxing Jan 10, 2015

Member

The alignment here seems a bit weird: since the ! only applies to post.excerpt_separator.empty?, the other two probably shouldn't be indented past the ! (in other words, maybe unindent the above 2 lines by 1 space).

@alfredxing

alfredxing Jan 10, 2015

Member

The alignment here seems a bit weird: since the ! only applies to post.excerpt_separator.empty?, the other two probably shouldn't be indented past the ! (in other words, maybe unindent the above 2 lines by 1 space).

lib/jekyll/post.rb
@@ -307,7 +315,7 @@ def extract_excerpt
end
def generate_excerpt?
- !(site.config['excerpt_separator'].to_s.empty?)
+ !site.config['excerpt_separator'].to_s.empty? || !excerpt_separator.empty?

This comment has been minimized.

@alfredxing

alfredxing Jan 10, 2015

Member

If we move the fallback into Post::excerpt_separator (see above comments), checking site.config won't be necessary here.

@alfredxing

alfredxing Jan 10, 2015

Member

If we move the fallback into Post::excerpt_separator (see above comments), checking site.config won't be necessary here.

This comment has been minimized.

@majioa

majioa Jan 10, 2015

Contributor

@alfredxing @parkr I've just made some changes to get separator proc.

@majioa

majioa Jan 10, 2015

Contributor

@alfredxing @parkr I've just made some changes to get separator proc.

This comment has been minimized.

@alfredxing

alfredxing Jan 10, 2015

Member

@majioa Looks good! Just one more comment below.

@alfredxing

alfredxing Jan 10, 2015

Member

@majioa Looks good! Just one more comment below.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 10, 2015

Member

Added a few comments in the code. Nice job on this!

Member

alfredxing commented Jan 10, 2015

Added a few comments in the code. Nice job on this!

+ #
+ # Returns the post excerpt_separator
+ def excerpt_separator
+ data.fetch('excerpt_separator') { site.config['excerpt_separator'].to_s }

This comment has been minimized.

@alfredxing

alfredxing Jan 10, 2015

Member

I think it would seem a bit simpler to write data.fetch('excerpt_separator', site.config['excerpt_separator'].to_s) instead of using the block.

@alfredxing

alfredxing Jan 10, 2015

Member

I think it would seem a bit simpler to write data.fetch('excerpt_separator', site.config['excerpt_separator'].to_s) instead of using the block.

This comment has been minimized.

@majioa

majioa Jan 10, 2015

Contributor

@alfredxing the form I've used just the same as the other fetchers. but I'd prefer the view: data['excerpt_separator'] || site.config['excerpt_separator'].to_s

@majioa

majioa Jan 10, 2015

Contributor

@alfredxing the form I've used just the same as the other fetchers. but I'd prefer the view: data['excerpt_separator'] || site.config['excerpt_separator'].to_s

This comment has been minimized.

@parkr

parkr Jan 10, 2015

Member

How come? They do different things.

@parkr

parkr Jan 10, 2015

Member

How come? They do different things.

This comment has been minimized.

@majioa

majioa Jan 11, 2015

Contributor

@parkr yes, but I believe in that context they will have the similar behaviour, because I think, no one will define excerpt_separator as a nil or false. And in my case the all the jekyll test results dont change the state after applying this change =)

@majioa

majioa Jan 11, 2015

Contributor

@parkr yes, but I believe in that context they will have the similar behaviour, because I think, no one will define excerpt_separator as a nil or false. And in my case the all the jekyll test results dont change the state after applying this change =)

This comment has been minimized.

@envygeeks

envygeeks Jan 14, 2015

Contributor

|| is better safe than sorry, people are people and people make mistakes, it's possible. Hash#fetch does not protect against basic cases of human editing, human assumption and human mistakes. Trust me, it happens, I've done it before.

@envygeeks

envygeeks Jan 14, 2015

Contributor

|| is better safe than sorry, people are people and people make mistakes, it's possible. Hash#fetch does not protect against basic cases of human editing, human assumption and human mistakes. Trust me, it happens, I've done it before.

This comment has been minimized.

@envygeeks

envygeeks Jan 14, 2015

Contributor

And by "done it before" I mean stripped out something leaving only the key, not thinking.

@envygeeks

envygeeks Jan 14, 2015

Contributor

And by "done it before" I mean stripped out something leaving only the key, not thinking.

This comment has been minimized.

@parkr

parkr Jan 15, 2015

Member

Whatever the solution, let's ensure this method always returns a string. Otherwise it'll blow up generate_excerpt?

@parkr

parkr Jan 15, 2015

Member

Whatever the solution, let's ensure this method always returns a string. Otherwise it'll blow up generate_excerpt?

This comment has been minimized.

@majioa

majioa Jan 15, 2015

Contributor

@parkr well, i'l fix the method to return string

@majioa

majioa Jan 15, 2015

Contributor

@parkr well, i'l fix the method to return string

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 14, 2015

Does this need to be here? Is there any explicit reason?

Does this need to be here? Is there any explicit reason?

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 17, 2015

It means you can use it in Liquid. Not crucial.

It means you can use it in Liquid. Not crucial.

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

1 check passed

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

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

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 17, 2015

Member

Thanks!

Member

parkr commented Jan 17, 2015

Thanks!

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