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

Minor refactors #1341

Merged
merged 16 commits into from Aug 30, 2013

Conversation

Projects
None yet
4 participants
@maul-esel
Contributor

maul-esel commented Jul 22, 2013

As the title suggests.

Some stuff that bugged me a for some time, some stuff CodeClimate complains about, some other minor stuff I stumbled upon.

Comments, additions, corrections are welcome.

further_data = Hash[ATTRIBUTES_FOR_LIQUID.map { |attribute|
[attribute, send(attribute)]
}]
data.deep_merge(further_data)

This comment has been minimized.

@maul-esel

maul-esel Jul 22, 2013

Contributor

I'd love to move this method into Convertible, but it won't know ATTRIBUTES_FOR_LIQUID then. Any suggestions?

This comment has been minimized.

@maul-esel

maul-esel Jul 22, 2013

Contributor

Figured it out.

end
# Public: the Post title, from the YAML Front-Matter or from the slug
#
# Returns the post title
def title
self.data["title"] || self.slug.split('-').select {|w| w.capitalize! || w }.join(' ')
self.data.fetch("title", self.slug.split('-').select {|w| w.capitalize! || w }.join(' '))

This comment has been minimized.

@maul-esel

maul-esel Jul 22, 2013

Contributor

I should probably move the slug stuff in another method, titleize_slug or something:

self.data.fetch("title", titleize_slug) 
@@ -127,7 +123,7 @@ def title
#
# Returns the path to the file relative to the site source
def path
self.data['path'] || File.join(@dir, '_posts', @name).sub(/\A\//, '')
self.data.fetch('path', File.join(@dir, '_posts', @name).sub(/\A\//, ''))

This comment has been minimized.

@maul-esel

maul-esel Jul 22, 2013

Contributor

All over jekyll, there's code to prepend / append a slash or to remove a leading / trailing slash (mostly from paths, of course).

I'm thinking about adding some kind of PathHelper class (or module?) for these methods. This could also handle the detection of destination within source (#1130) and possibly others (#1297?, #1184?). Also, my PR for default layouts #969 also works with this kind of stuff.

Good / bad idea? Class / Module / extension to String?

This comment has been minimized.

@mattr-

mattr- Jul 22, 2013

Member

Sounds good. Let's see some code. 😃

This comment has been minimized.

@maul-esel

maul-esel Jul 22, 2013

Contributor

Just not sure what form to put it in. Could be a module, as Pager has them as static methods? Or a class, because I'd find Post#remove_leading_slash(path) a little confusing...

This comment has been minimized.

@mattr-

mattr- Jul 22, 2013

Member

Class or Module shouldn't really matter. We just need some place to hang
the methods.

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

Module would be better, though I'd prefer not to include it, but rather call Paths.remove_leading_slash(path).

This comment has been minimized.

@maul-esel

maul-esel Jul 26, 2013

Contributor

Out of curiousity, what's the difference between a class with static methods and a module with static methods?

This comment has been minimized.

@maul-esel

maul-esel Jul 29, 2013

Contributor

Either way, I'd probably put that off to another PR at a later point, if that's OK for you?

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jul 22, 2013

This does actually build fine, just some random issue here.

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 22, 2013

Seems fine to me. @parkr?

maul-esel added some commits Jul 23, 2013

remove no-longer needed LSI accessor
LSI has been moved to another class (Jekyll::RelatedPosts),
but this was left in Post.
refactor Include tag file validation
Split validation into a separate method, and give a more
descriptive error on symlinks.
})
%w[wrap line_numbers line_numbers_start tab_width bold_every css default_lang].each do |opt|
key = "coderay_#{opt}"
@config['kramdown'][key.to_sym] = @config['kramdown']['coderay'][key] unless @config['kramdown'].has_key? key

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

Would you please add parentheses around the arg to the #has_key? call here?

This comment has been minimized.

@maul-esel

maul-esel Jul 26, 2013

Contributor

Done!

#
# Returns the Hash representation of this Convertible.
def to_liquid
further_data = Hash[self.class::ATTRIBUTES_FOR_LIQUID.map { |attribute|

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

I would prefer to have this passed as an argument than to see the self.class::. My PR #1339 will change this in Post.

This comment has been minimized.

@maul-esel

maul-esel Jul 24, 2013

Contributor

Problem is, it needs to default to self.class::ATTRIBUTES_FOR_LIQUID, as Liquid seemingly calls it internally and passes no params to it.

This comment has been minimized.

@maul-esel

maul-esel Jul 29, 2013

Contributor

I made it default to self.class::ATTRIBUTES_FOR_LIQUID now, as it needs a default, but with the ability to override the attributes.

# The path to the source file
#
# Returns the path to the source file
def path
File.join(@dir, @name).sub(/\A\//, '')
self.data.fetch('path', File.join(@dir, @name).sub(/\A\//, ''))

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

Maybe default_path for the default up there?

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

Or relative_path

This comment has been minimized.

@maul-esel

maul-esel Jul 26, 2013

Contributor

Done!

end
# Turns the post slug into a suitable title
def titleize_slug

This comment has been minimized.

@parkr

parkr Jul 23, 2013

Member

titleized_slug would be better here, I think. Methods that don't take args should describe a condition (e.g. "titleized") whereas methods that do take arguments should describe an action (e.g. "titleize").

This comment has been minimized.

@maul-esel

maul-esel Jul 26, 2013

Contributor

Done!

@parkr

This comment has been minimized.

Member

parkr commented Jul 23, 2013

Thanks for this refactoring! Definitely need to clean up this code.

@mattr-

This comment has been minimized.

Member

mattr- commented Jul 24, 2013

Looks like it needs a rebase, stat! 😉

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jul 29, 2013

Should be ready to go now!

@mattr-

This comment has been minimized.

Member

mattr- commented Aug 7, 2013

👍 from me. @parkr you ok with this update?

:coderay_bold_every => @config['kramdown']['coderay']['coderay_bold_every'],
:coderay_css => @config['kramdown']['coderay']['coderay_css']
})
%w[wrap line_numbers line_numbers_start tab_width bold_every css default_lang].each do |opt|

This comment has been minimized.

@parkr

parkr Aug 7, 2013

Member

What is the reason for adding coderay_default_lang?

This comment has been minimized.

@maul-esel

maul-esel Aug 17, 2013

Contributor

Completeness.

This comment has been minimized.

@parkr

parkr Aug 17, 2013

Member

Fair enough!

@@ -51,6 +51,21 @@ def validate_syntax
def render(context)
includes_dir = File.join(context.registers[:site].source, '_includes')
error = self.validate_file(includes_dir)

This comment has been minimized.

@parkr

parkr Aug 7, 2013

Member

why the self. here?

This comment has been minimized.

@maul-esel

maul-esel Aug 17, 2013

Contributor

I wasn't sure as to what's the preferred coding style. I'll remove it.

This comment has been minimized.

@parkr

parkr Aug 17, 2013

Member

We tend to use self only for #attribute= and #attribute methods.

error = self.validate_file(includes_dir)
unless error.nil?
return error
end

This comment has been minimized.

@parkr

parkr Aug 7, 2013

Member

what about

return error if error = validate_file(includes_dir)

?

end
nil

This comment has been minimized.

@parkr

parkr Aug 7, 2013

Member

nil is automatically returned - no need to do that explicitly here!

This comment has been minimized.

@maul-esel

maul-esel Aug 17, 2013

Contributor

I always prefer to be explicit about return values - mostly because some languages I used are not so explicit about the default, secondly to indicate that nil as return value has some meaning here (as a reminder in case the code is changed later).

But nevertheless, it's kinda redundant so I'll remove it.

if !File.exists?(file)
return "Included file #{@file} not found in _includes directory"
elsif File.symlink?(file)
return "Included file #{@file} is a symlink"

This comment has been minimized.

@parkr

parkr Aug 7, 2013

Member

how will i know that this is not allowed?

@parkr

This comment has been minimized.

Member

parkr commented Aug 7, 2013

A few more comments :)

improve error handling in include tag
* Reduce condition to one-liner
* Remove `self`
* Implicit return value
* Explicit error message for symlinks
@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Aug 17, 2013

Adressed your comments. Let me know if there's anything else to do. And sorry for the delay, I was out of the country and far from any computers ✈️ 🇨🇳

@parkr

This comment has been minimized.

Member

parkr commented Aug 17, 2013

@maul-esel Welcome back! Hope you had fun in China. Glad to see you hanging around Jekyll again :)

@parkr

This comment has been minimized.

Member

parkr commented Aug 17, 2013

@mattr-, would you mind taking another look? LGTM.

@mattr-

This comment has been minimized.

mattr- commented on lib/jekyll/tags/include.rb in 95719fa Aug 18, 2013

I think The included file #{@file} should not be a symlink would be a better error message here.

This comment has been minimized.

mattr- replied Aug 18, 2013

Also, should we use file here (without the @) in order to display the full path to the file?

This comment has been minimized.

Owner

maul-esel replied Aug 18, 2013

I agree on the reworded message, that sounds much better.

However, I'm not sure concerning the full path. Of what help is the path to the site root in this context? I'd rather use _includes/#{@file}, but even that is a little redundant.

This comment has been minimized.

mattr- replied Aug 18, 2013

This comment has been minimized.

Owner

maul-esel replied Aug 18, 2013

☑️

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

I approve of this message 🐫

LGTM. @mattr-?

@ghost ghost assigned mattr- Aug 26, 2013

mattr- added a commit that referenced this pull request Aug 30, 2013

@mattr- mattr- merged commit a9e2a74 into jekyll:master Aug 30, 2013

1 check passed

default The Travis CI build passed
Details
@mattr-

This comment has been minimized.

Member

mattr- commented Aug 30, 2013

🎉 🚢ed

Nice work! Thank you so much for sticking with it. 😃

mattr- added a commit that referenced this pull request Aug 30, 2013

@maul-esel maul-esel deleted the maul-esel:minor-refactors branch Aug 30, 2013

@parkr

This comment has been minimized.

Member

parkr commented Aug 30, 2013

Thanks @mattr-!

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