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

Fix `jekyll serve --limit_posts n` failed #1004

Merged
merged 4 commits into from Apr 30, 2013

Conversation

Projects
None yet
5 participants
@uu59
Contributor

uu59 commented Apr 27, 2013

Just fix this error:

$ ./bin/jekyll serve --limit_posts 2 --trace
Configuration file: /home/nyaran/blog.uu59.org/_config.yml
/home/nyaran/.rbenv/versions/1.9.3-p374/lib/ruby/gems/1.9.1/bundler/gems/jekyll-c0c0150bb5a3/lib/jekyll/site.rb:65:in `<': comparison of String with 1 failed (ArgumentError)

@parkr

This comment has been minimized.

Member

parkr commented Apr 28, 2013

Thanks for bringing up this problem! I think this is better served by specifying that the --limit_posts switch take an Integer (via Commander in bin/jekyll) and set a default of 0 in the Configuration class.

Change default value of limit_posts from nil to 0 (see #1004)
Before this commit meaning of limit_posts:
  nil: no limit. generate all posts
  0: raise error
  n ( > 0): generate n posts only
  n ( < 0): raise error
  else: raise error

After this commit:
  nil: same as 0
  0: no limit. generate all posts
  n ( > 0): generate n posts only
  n ( < 0): raise error
  else: almost same as 0 (depend on `to_i` result)
@uu59

This comment has been minimized.

Contributor

uu59 commented Apr 28, 2013

Updated.
How do you think?

@@ -27,7 +27,8 @@ def initialize(config)
self.include = config['include']
self.future = config['future']
self.show_drafts = config['show_drafts']
self.limit_posts = config['limit_posts']
# given as String if it is came from CLI option

This comment has been minimized.

@parkr

parkr Apr 28, 2013

Member

change bin/jekyll such that the --limit_posts flag accepts an integer. then the .to_i won't be needed because it'll parse into an integer without any need for extra code on our part here.

@uu59

This comment has been minimized.

Contributor

uu59 commented Apr 29, 2013

Sorry I wrote a wrong issue number on b2d89c9, so push -f-ed.

@@ -148,7 +148,7 @@ def read_directories(dir = '')
self.posts.sort!
# limit the posts if :limit_posts option is set
if limit_posts
unless limit_posts.zero?

This comment has been minimized.

@parkr

parkr Apr 29, 2013

Member

the line you changed should have worked, right? 0 evaluates to false if i remember correctly, and it looks like you take care of values less than 0 above so you should always have a value of 0 or greater.

This comment has been minimized.

@uu59

uu59 Apr 29, 2013

Contributor

No, 0 is not falsy such as puts "hi" if 0 # => hi. nil or false are the only values evaluated as false :)

But I agree that unless limit_posts.zero? is not clearly condition. I'll change it as if limit_posts > 0

This comment has been minimized.

@parkr

parkr Apr 29, 2013

Member

well that's stupid. oh, ruby. ;)

if !self.limit_posts.nil? && self.limit_posts < 1
raise ArgumentError, "Limit posts must be nil or >= 1"
if self.limit_posts < 0
raise ArgumentError, "Limit posts must be nil or >= 0"

This comment has been minimized.

@parkr

parkr Apr 29, 2013

Member

Good catch! Why don't we change this to Jekyll::Logger.error("Argument Error:", "limit_posts must be >= 0")

This comment has been minimized.

@parkr

parkr Apr 29, 2013

Member

then you can raise an argument error. it's all about the formatting of the output :)

@uu59

This comment has been minimized.

Contributor

uu59 commented Apr 29, 2013

Thank you for your respectful review :)

@parkr

This comment has been minimized.

Member

parkr commented Apr 30, 2013

LGTM! @mattr-, what do you think?

@mattr-

This comment has been minimized.

Member

mattr- commented Apr 30, 2013

👍 :shipit:

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

Merge pull request #1004 from uu59/fix_limit_posts_from_cli
Fix `jekyll serve --limit_posts n` failed

@parkr parkr merged commit 7efd0a8 into jekyll:master Apr 30, 2013

1 check passed

default The Travis build passed
Details

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

@essia

This comment has been minimized.

essia commented on 9475634 May 8, 2013

welome

This comment has been minimized.

Member

parkr replied May 8, 2013

?

@essia

This comment has been minimized.

essia commented on bin/jekyll in 9475634 May 8, 2013

c.option '--limit_posts MAX_POSTS', 'Limits the number of posts to parse and publish

@essia

This comment has been minimized.

essia commented on bin/jekyll in 9475634 May 8, 2013

Integer,

This comment has been minimized.

essia replied May 8, 2013

  • c.option '--limit_posts MAX_POSTS', Integer, 'Limits the number of posts to parse and publish'

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