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

Proxy a number of Convertible methods to Renderer #5308

Merged
merged 10 commits into from Sep 2, 2016

Conversation

Projects
None yet
5 participants
@parkr
Member

parkr commented Aug 30, 2016

This simplifies the code a bit -- much less duplication.

We might consider providing a way to set the layouts for a Renderer so we can override it if necessary. It seems a bit aggressive to want/have to change site.layouts in order for this to work.
#4885

parkr added some commits Aug 30, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

I am indebted to #5052 which gave me the idea for this.

Member

parkr commented Aug 30, 2016

I am indebted to #5052 which gave me the idea for this.

parkr added some commits Aug 30, 2016

Show outdated Hide outdated lib/jekyll/renderer.rb
def initialize(site, document, site_payload = nil)
def initialize(site, document, site_payload = nil, layouts: nil)

This comment has been minimized.

@parkr

parkr Sep 1, 2016

Member

@envygeeks Are named parameters not allowed in jruby? https://travis-ci.org/jekyll/jekyll/jobs/156357192

@parkr

parkr Sep 1, 2016

Member

@envygeeks Are named parameters not allowed in jruby? https://travis-ci.org/jekyll/jekyll/jobs/156357192

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 1, 2016

Member

Works! Turns out jruby doesn't like named parameters. jruby/jruby#1403

Member

parkr commented Sep 1, 2016

Works! Turns out jruby doesn't like named parameters. jruby/jruby#1403

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Sep 1, 2016

Contributor

Do you mean keywords @parkr? Because JRuby has full support for keywords. What error were you getting?

Contributor

envygeeks commented Sep 1, 2016

Do you mean keywords @parkr? Because JRuby has full support for keywords. What error were you getting?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 1, 2016

Member

@envygeeks Yeah, I thought that too and the error was weird. The build I saw it in was https://travis-ci.org/jekyll/jekyll/jobs/156357192 and it was fixed by 4af0f02.

Member

parkr commented Sep 1, 2016

@envygeeks Yeah, I thought that too and the error was weird. The build I saw it in was https://travis-ci.org/jekyll/jekyll/jobs/156357192 and it was fixed by 4af0f02.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Sep 1, 2016

Contributor

I'll look at the error but what I do know is we use keywords in JRuby (we
mostly deploy with JRuby.)

On Thu, Sep 1, 2016, 6:13 PM Parker Moore notifications@github.com wrote:

@envygeeks https://github.com/envygeeks Yeah, I thought that too and
the error was weird. The build I saw it in was
https://travis-ci.org/jekyll/jekyll/jobs/156357192 and it was fixed by
4af0f02
4af0f02
.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#5308 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGFs0ptB585hnIHNmjXNg4T4JM-UtGHks5ql1wZgaJpZM4Jw_On
.

Contributor

envygeeks commented Sep 1, 2016

I'll look at the error but what I do know is we use keywords in JRuby (we
mostly deploy with JRuby.)

On Thu, Sep 1, 2016, 6:13 PM Parker Moore notifications@github.com wrote:

@envygeeks https://github.com/envygeeks Yeah, I thought that too and
the error was weird. The build I saw it in was
https://travis-ci.org/jekyll/jekyll/jobs/156357192 and it was fixed by
4af0f02
4af0f02
.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#5308 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGFs0ptB585hnIHNmjXNg4T4JM-UtGHks5ql1wZgaJpZM4Jw_On
.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 2, 2016

Member

@jekyllbot: merge +dev

Member

parkr commented Sep 2, 2016

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 2f167ae into master Sep 2, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the rubocop-convertible branch Sep 2, 2016

jekyllbot added a commit that referenced this pull request Sep 2, 2016

Crunch09 added a commit to Crunch09/jekyll that referenced this pull request Sep 6, 2016

Revert "Merge pull request #5308 from jekyll/rubocop-convertible"
This reverts commit 2f167ae, reversing
changes made to 4f80dde.
@joallard

This comment has been minimized.

Show comment
Hide comment
@joallard

joallard Nov 2, 2016

I'm hunting down jekyll-haml#19, and I don't get this one. How can we call Renderer#transform if there's no such method?

Edit: Looks like #5052 has a fix for that. On to another error...

joallard commented on d0f57b6 Nov 2, 2016

I'm hunting down jekyll-haml#19, and I don't get this one. How can we call Renderer#transform if there's no such method?

Edit: Looks like #5052 has a fix for that. On to another error...

This comment has been minimized.

Show comment
Hide comment
@svpersteve

svpersteve Dec 3, 2016

@joallard does this mean #5052 will fix samvincent/jekyll-haml#19 but it just hasn't been merge yet? Do you know if it's the only bug preventing jekyll-haml working with the latest version of Jekyll?

svpersteve replied Dec 3, 2016

@joallard does this mean #5052 will fix samvincent/jekyll-haml#19 but it just hasn't been merge yet? Do you know if it's the only bug preventing jekyll-haml working with the latest version of Jekyll?

end
# Determine which converter to use based on this convertible's
# extension.
#
# Returns the Converter instance.
def converters
@converters ||= site.converters.select { |c| c.matches(ext) }.sort

This comment has been minimized.

@joallard

joallard Nov 2, 2016

If Convertible is a Layout, .extname will be undefined, as called by Renderer.

[36] pry(#<Jekyll::Layout>)> _renderer.convert(content)
NoMethodError: undefined method `extname' for #<Jekyll::Layout:0x007fa8c3cec048>
from /Users/jon/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/jekyll-3.3.0/lib/jekyll/renderer.rb:38:in `block in converters'
[37] pry(#<Jekyll::Layout>)> frame 0

Frame number: 0/20
Frame type: block

From: /Users/jon/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/jekyll-3.3.0/lib/jekyll/renderer.rb @ line 38 Jekyll::Renderer#converters:

    37: def converters
 => 38:   @converters ||= site.converters.select { |c| c.matches(document.extname) }.sort
    39: end

[38] pry(#<Jekyll::Renderer>)> document.class
=> Jekyll::Layout
[39] pry(#<Jekyll::Renderer>)> document.ext
=> ".html"
[40] pry(#<Jekyll::Renderer>)> document.extname
NoMethodError: undefined method `extname' for #<Jekyll::Layout:0x007fa8c3cec048>
from (pry):20:in `block in converters'
@joallard

joallard Nov 2, 2016

If Convertible is a Layout, .extname will be undefined, as called by Renderer.

[36] pry(#<Jekyll::Layout>)> _renderer.convert(content)
NoMethodError: undefined method `extname' for #<Jekyll::Layout:0x007fa8c3cec048>
from /Users/jon/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/jekyll-3.3.0/lib/jekyll/renderer.rb:38:in `block in converters'
[37] pry(#<Jekyll::Layout>)> frame 0

Frame number: 0/20
Frame type: block

From: /Users/jon/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/jekyll-3.3.0/lib/jekyll/renderer.rb @ line 38 Jekyll::Renderer#converters:

    37: def converters
 => 38:   @converters ||= site.converters.select { |c| c.matches(document.extname) }.sort
    39: end

[38] pry(#<Jekyll::Renderer>)> document.class
=> Jekyll::Layout
[39] pry(#<Jekyll::Renderer>)> document.ext
=> ".html"
[40] pry(#<Jekyll::Renderer>)> document.extname
NoMethodError: undefined method `extname' for #<Jekyll::Layout:0x007fa8c3cec048>
from (pry):20:in `block in converters'

This comment has been minimized.

@parkr

parkr Mar 31, 2017

Member

Hey! It's been ages since you commented, but I'm circling back now. A new PR (or an issue) will definitely get more of a response than a comment on a really old PR. But here we are!

So you're saying we took away extname and everything exploded?

@parkr

parkr Mar 31, 2017

Member

Hey! It's been ages since you commented, but I'm circling back now. A new PR (or an issue) will definitely get more of a response than a comment on a really old PR. But here we are!

So you're saying we took away extname and everything exploded?

This comment has been minimized.

@joallard

joallard Apr 3, 2017

Hey @parkr, sorry about that! I was on the hunt for what prevented Jekyll-Haml from working, but I've moved on since. Looks like they ended up finding a way!

@joallard

joallard Apr 3, 2017

Hey @parkr, sorry about that! I was on the hunt for what prevented Jekyll-Haml from working, but I've moved on since. Looks like they ended up finding a way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment