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

Preserve excerpt #946

Merged
merged 8 commits into from Apr 12, 2013

Conversation

Projects
None yet
4 participants
@maul-esel
Copy link
Contributor

commented Apr 11, 2013

This addresses #933: do not override a custom excerpt.

@mattr-

This comment has been minimized.

Copy link
Member

commented Apr 11, 2013

👍 Thanks for doing this! ❤️ :shipit: 🚢

assert_equal("<p>I can set a custom excerpt with <em>markdown</em></p>", @post.excerpt)
end

end

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

formatting got a little weird here...

This comment has been minimized.

Copy link
@mattr-

mattr- Apr 11, 2013

Member

oh, so it did. totally missed that (and i'm usually the formatting nitpicker 😉)

@@ -80,7 +80,7 @@ def containing_dir(source, dir)
# Returns nothing.
def read_yaml(base, name)
super(base, name)
self.excerpt = self.extract_excerpt
self.excerpt = self.data["excerpt"] || self.extract_excerpt

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

i think i'd rather have a self.excerpt accessor method which returns data["excerpt"] if it exists, or falls back on result of #extract_excerpt, maybe extracted_excerpt?

maul-esel added some commits Apr 11, 2013

rework excerpt to be an accessor method
Instead of setting self.excerpt, make it a method
that returns either the custom excerpt or the pre-
viously extracted excerpt.
#
# Returns excerpt string.
def excerpt
self.data['excerpt'] ? converter.convert(self.data['excerpt']) : self.extracted_excerpt

This comment has been minimized.

Copy link
@maul-esel

maul-esel Apr 11, 2013

Author Contributor

Should I also move the conversion of self.data['excerpt'] to transform or is it fine that way? Was not sure if one is supposed to modify self.data...

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

Why are you converting self.data['excerpt']?

This comment has been minimized.

Copy link
@maul-esel

maul-esel Apr 11, 2013

Author Contributor

To allow basic markdown etc. (as in the first test) in the excerpt.

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

I'd do something like

if self.data.has_key? "excerpt"
  self.data['excerpt']
else
  self.extracted_excerpt
end

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

I don't think that's a good idea. I'd prefer not to convert the excerpt! The excerpt in your front-matter shouldn't be long or complex enough to warrant conversion.

This comment has been minimized.

Copy link
@maul-esel

maul-esel Apr 11, 2013

Author Contributor

Done!

This comment has been minimized.

Copy link
@mattr-

mattr- Apr 11, 2013

Member

We convert the extracted excerpt. Why not convert the excerpt that's in the
YAML front matter?

On Thu, Apr 11, 2013 at 2:03 PM, maul-esel notifications@github.com wrote:

In lib/jekyll/post.rb:

   self.data['layout'] = 'post' unless self.data.has_key?('layout')
 end
  • The post excerpt. This is either a custom excerpt

  • set in YAML front matter or the result of extract_excerpt.

  • Returns excerpt string.

  • def excerpt
  •  self.data['excerpt'] ? converter.convert(self.data['excerpt']) : self.extracted_excerpt
    

Done!


Reply to this email directly or view it on GitHubhttps://github.com//pull/946/files#r3759594
.

This comment has been minimized.

Copy link
@parkr

parkr Apr 12, 2013

Member

It will modify the current expected behaviour for one, and I feel like it shouldn't be so long as to require any sort of markdown format. The former reason is the reason I'm to stick with, though :)

This comment has been minimized.

Copy link
@mattr-

mattr- Apr 12, 2013

Member

I could see people wanting to have a bit of emphasis on their extracted excerpt, but I don't feel strongly enough about it to be all like "no, we shouldn't merge this, we gotta have converted excerpts". We can just add it later if it gets asked for. 😀

attr_accessor :date, :slug, :published, :tags, :categories

attr_reader :name
attr_reader :name, :excerpt

This comment has been minimized.

Copy link
@parkr

parkr Apr 11, 2013

Member

this shouldn't be here if you have the method below, right? :)

@parkr

This comment has been minimized.

Copy link
Member

commented Apr 11, 2013

Can you please fill out the unit tests a bit more? It only covers one case!

@maul-esel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2013

@parkr: I added one more test (ensure the correct excerpt is exposed), but I'm not sure what else to test - the "regular" excerpt behaviour is already tested.

do not allow markdown etc. in excerpt
Adjust the tests accordingly. Also
add a message for one of the tests.
# Returns excerpt string.
def excerpt
if self.data.has_key? 'excerpt'
self.data['excerpt']

This comment has been minimized.

Copy link
@parkr

parkr Apr 12, 2013

Member

indent error here and below :) sorry!

parkr added a commit that referenced this pull request Apr 12, 2013

Merge pull request #946 from maul-esel/preserve_excerpt
Preserve 'excerpt` in YAML Front-Matter

@parkr parkr merged commit bee8cd9 into jekyll:master Apr 12, 2013

1 check passed

default The Travis build passed
Details

@maul-esel maul-esel deleted the maul-esel:preserve_excerpt branch Apr 12, 2013

parkr added a commit that referenced this pull request Apr 12, 2013

@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.
You can’t perform that action at this time.