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

rubocop: lib/jekyll/renderer.rb complexity fixes #5052

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@ayastreb
Contributor

ayastreb commented Jul 3, 2016

#4885

In this PR I refactored Renderer class, removed duplicated code from Convertible and changed Page class to use Renderer.run instead of Convertible.do_layout

Show outdated Hide outdated lib/jekyll/renderer.rb
#
# Returns nothing
private
# rubocop: disable AbcSize

This comment has been minimized.

@ayastreb

ayastreb Jul 3, 2016

Contributor

The ABC metric is 25 in this method (20 is max).
I couldn't think of a way to reduce it without creating too many helper methods.
Any suggestions? 😄

@ayastreb

ayastreb Jul 3, 2016

Contributor

The ABC metric is 25 in this method (20 is max).
I couldn't think of a way to reduce it without creating too many helper methods.
Any suggestions? 😄

@parkr parkr self-assigned this Aug 24, 2016

@pathawks pathawks referenced this pull request Aug 25, 2016

Closed

Make jekyll/jekyll code compliant with rubocop rules #4885

114 of 115 tasks complete
@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Aug 25, 2016

Contributor

I've rebased this PR to latest master and fixed conflicts.

Contributor

ayastreb commented Aug 25, 2016

I've rebased this PR to latest master and fixed conflicts.

Show outdated Hide outdated lib/jekyll/page.rb
do_layout(site_payload, layouts)
site.layouts = layouts
Renderer.new(site, self, site_payload).run

This comment has been minimized.

@ayastreb

ayastreb Aug 25, 2016

Contributor

@benbalter is doing similar change in this method here, but skipping layouts variable.
Should we do the same here?

@ayastreb

ayastreb Aug 25, 2016

Contributor

@benbalter is doing similar change in this method here, but skipping layouts variable.
Should we do the same here?

Show outdated Hide outdated lib/jekyll/page.rb
site_payload["paginator"] = pager.to_liquid
do_layout(site_payload, layouts)
site.layouts = layouts

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

This is a no-go. We don't want to reassign site.layouts.

@parkr

parkr Aug 30, 2016

Member

This is a no-go. We don't want to reassign site.layouts.

This comment has been minimized.

@ayastreb

ayastreb Aug 30, 2016

Contributor

Ok, should we just ignore the layouts argument then?

@ayastreb

ayastreb Aug 30, 2016

Contributor

Ok, should we just ignore the layouts argument then?

Show outdated Hide outdated lib/jekyll/renderer.rb
def converters
@converters ||= site.converters.select { |c| c.matches(document.extname) }
@converters ||= site.converters.select { |c| c.matches(document.extname) }.sort

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

Might break things but technically a bug.

@parkr

parkr Aug 30, 2016

Member

Might break things but technically a bug.

This comment has been minimized.

@ayastreb

ayastreb Aug 30, 2016

Contributor

Hmm, that came from Convertible 😄
What would you suggest to do?

@ayastreb

ayastreb Aug 30, 2016

Contributor

Hmm, that came from Convertible 😄
What would you suggest to do?

Show outdated Hide outdated lib/jekyll/renderer.rb
if document.render_with_liquid?
Jekyll.logger.debug "Rendering Liquid:", document.relative_path
output = render_liquid(output, payload, info, document.path)
document.output = render_liquid(document.content, document.path)

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

Assigning document.output should not be done here – we should not modify document inside the renderer; it is up to the caller to modify document.

@parkr

parkr Aug 30, 2016

Member

Assigning document.output should not be done here – we should not modify document inside the renderer; it is up to the caller to modify document.

Show outdated Hide outdated lib/jekyll/convertible.rb
# Transform the contents based on the content type.
#
# Returns the transformed contents.
def transform

This comment has been minimized.

@parkr

parkr Aug 30, 2016

Member

We can't remove these from Convertible until Jekyll 4 (when we'll remove the whole module). These could proxy out to a Jekyll::Renderer instance created in a private helper method.

@parkr

parkr Aug 30, 2016

Member

We can't remove these from Convertible until Jekyll 4 (when we'll remove the whole module). These could proxy out to a Jekyll::Renderer instance created in a private helper method.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

Thanks for this and sorry for the delay in reviewing. This appears to change too many things with the actual functionality and compatibility so I'm not going to merge it. Do you want to fix it up based on my comments? I could also accept a PR which just proxied the Convertible method calls to a Jekyll::Renderer instance then take on the renderer changes later.

Member

parkr commented Aug 30, 2016

Thanks for this and sorry for the delay in reviewing. This appears to change too many things with the actual functionality and compatibility so I'm not going to merge it. Do you want to fix it up based on my comments? I could also accept a PR which just proxied the Convertible method calls to a Jekyll::Renderer instance then take on the renderer changes later.

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Aug 30, 2016

Contributor

@parkr
Sure, I can open new PR after #5308 is merged and will only fix Renderer complexity issues there. Let's keep this PR open until then?

Contributor

ayastreb commented Aug 30, 2016

@parkr
Sure, I can open new PR after #5308 is merged and will only fix Renderer complexity issues there. Let's keep this PR open until then?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 2, 2016

Member

Merged! Would you mind rebasing?

Member

parkr commented Sep 2, 2016

Merged! Would you mind rebasing?

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Sep 2, 2016

Contributor

@parkr done!

Contributor

ayastreb commented Sep 2, 2016

@parkr done!

Show outdated Hide outdated lib/jekyll/convertible.rb
@@ -193,7 +193,7 @@ def invalid_layout?(layout)
# Returns nothing
def render_all_layouts(layouts, payload, info)
_renderer.layouts = layouts
_renderer.place_in_layouts(output, payload, info)
_renderer.place_in_layouts(payload, info)

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

We can't change any API signatures during a rubocop refactor. We don't know what plugins use this method and don't want to break them. Please revert this change!

@parkr

parkr Sep 8, 2016

Member

We can't change any API signatures during a rubocop refactor. We don't know what plugins use this method and don't want to break them. Please revert this change!

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Sep 8, 2016

Contributor

@parkr ok, I've reverted all signature changes

Contributor

ayastreb commented Sep 8, 2016

@parkr ok, I've reverted all signature changes

Show outdated Hide outdated lib/jekyll/page.rb
site_payload["paginator"] = pager.to_liquid
do_layout(site_payload, layouts)
renderer = Renderer.new(site, self, site_payload)

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

This class still includes Convertible. You should use the _renderer method here instead and just set payload and layouts. Like:

self.output = _renderer.tap do |renderer|
  renderer.payload = site_payload
  renderer.layouts = layouts
end.run
@_renderer = nil
self.output

:)

@parkr

parkr Sep 8, 2016

Member

This class still includes Convertible. You should use the _renderer method here instead and just set payload and layouts. Like:

self.output = _renderer.tap do |renderer|
  renderer.payload = site_payload
  renderer.layouts = layouts
end.run
@_renderer = nil
self.output

:)

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

Ah, I did something like this in #5337.

@parkr

parkr Sep 8, 2016

Member

Ah, I did something like this in #5337.

Show outdated Hide outdated lib/jekyll/renderer.rb
payload["highlighter_suffix"] = converters.first.highlighter_suffix
assign_pages
assign_related_posts
assign_highlighter_options

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

I'd love for these to each be "bang" methods because they modify the payload:

assign_pages!
assign_related_posts!
assign_highlighter_options!
@parkr

parkr Sep 8, 2016

Member

I'd love for these to each be "bang" methods because they modify the payload:

assign_pages!
assign_related_posts!
assign_highlighter_options!
info = {
:filters => [Jekyll::Filters],

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

I think we removed this on purpose somewhere... Why did you add it back out of curiosity?

@parkr

parkr Sep 8, 2016

Member

I think we removed this on purpose somewhere... Why did you add it back out of curiosity?

This comment has been minimized.

@ayastreb

ayastreb Sep 8, 2016

Contributor

I think I copied it over from Convertible, it was used there -https://github.com/jekyll/jekyll/pull/5308/files#diff-817b22aa9906612f8c43543d6aa8202dL262

should it be removed? why was it used in Convertible? 😄

@ayastreb

ayastreb Sep 8, 2016

Contributor

I think I copied it over from Convertible, it was used there -https://github.com/jekyll/jekyll/pull/5308/files#diff-817b22aa9906612f8c43543d6aa8202dL262

should it be removed? why was it used in Convertible? 😄

Show outdated Hide outdated lib/jekyll/renderer.rb
output = document.content
@output = document.content

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

I'd rather not keep this as an instance variable. Is it used outside the scope and not passed? I think just output would be better and to just pass it to the methods that need it. Less state for this class we have to reason about the better. 😄

@parkr

parkr Sep 8, 2016

Member

I'd rather not keep this as an instance variable. Is it used outside the scope and not passed? I think just output would be better and to just pass it to the methods that need it. Less state for this class we have to reason about the better. 😄

This comment has been minimized.

@ayastreb

ayastreb Sep 8, 2016

Contributor

Hmm... I think it made more sense before when I changed the signatures.
But since we can't change the signatures, I'll roll it back to just pass it to the methods!

@ayastreb

ayastreb Sep 8, 2016

Contributor

Hmm... I think it made more sense before when I changed the signatures.
But since we can't change the signatures, I'll roll it back to just pass it to the methods!

Show outdated Hide outdated lib/jekyll/renderer.rb
def place_in_layouts(content, payload, info)
output = content.dup
@output = content.dup

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

Same here – just output would be better in my opinion.

@parkr

parkr Sep 8, 2016

Member

Same here – just output would be better in my opinion.

Show outdated Hide outdated lib/jekyll/renderer.rb
if (layout = layouts[layout.data["layout"]])
render_layout(layout, info)
add_layout_to_dependency(layout)

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

Let's rephrase this method – I was a bit confused by its name. What do you think about add_regenerator_dependencies(layout)?

@parkr

parkr Sep 8, 2016

Member

Let's rephrase this method – I was a bit confused by its name. What do you think about add_regenerator_dependencies(layout)?

Show outdated Hide outdated lib/jekyll/renderer.rb
# Returns nothing
private
def render_layout(layout, info)
payload["content"] = @output

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

I think passing the value of output as an argument would be easier to reason about.

@parkr

parkr Sep 8, 2016

Member

I think passing the value of output as an argument would be easier to reason about.

Show outdated Hide outdated lib/jekyll/renderer.rb
payload["content"] = @output
payload["layout"] = Utils.deep_merge_hashes(layout.data, payload["layout"] || {})
@output = render_liquid(

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

And then returning it here instead of using the instance variable.

@parkr

parkr Sep 8, 2016

Member

And then returning it here instead of using the instance variable.

Show outdated Hide outdated lib/jekyll/renderer.rb
#
# Returns nothing
private
def assign_pages

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

assign_pages!

@parkr

parkr Sep 8, 2016

Member

assign_pages!

Show outdated Hide outdated lib/jekyll/renderer.rb
#
# Returns nothing
private
def assign_related_posts

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

assign_related_posts!

@parkr

parkr Sep 8, 2016

Member

assign_related_posts!

Show outdated Hide outdated lib/jekyll/renderer.rb
def assign_pages
payload["page"] = document.to_liquid
if document.respond_to?(:pager)
payload["paginator"] = document.pager.to_liquid

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

Let's set the paginator to nil if the document doesn't response, just like we do in the similar situation of assign_related_posts!.

@parkr

parkr Sep 8, 2016

Member

Let's set the paginator to nil if the document doesn't response, just like we do in the similar situation of assign_related_posts!.

Show outdated Hide outdated lib/jekyll/renderer.rb
private
def assign_highlighter_options

This comment has been minimized.

@parkr

parkr Sep 8, 2016

Member

assign_highlighter_options!

@parkr

parkr Sep 8, 2016

Member

assign_highlighter_options!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 8, 2016

Member

Whew! OK, I left all my comments. This is looking pretty good! Let me know if you don't want to make these edits and I can merge your PR locally, make the edits, and push to master. 👍

Member

parkr commented Sep 8, 2016

Whew! OK, I left all my comments. This is looking pretty good! Let me know if you don't want to make these edits and I can merge your PR locally, make the edits, and push to master. 👍

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 8, 2016

Member

Also: thank you! ❤️

Member

parkr commented Sep 8, 2016

Also: thank you! ❤️

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Sep 8, 2016

Contributor

Hey @parkr ,
thanks for the feedback! I will try to do the edits tomorrow 😄

Contributor

ayastreb commented Sep 8, 2016

Hey @parkr ,
thanks for the feedback! I will try to do the edits tomorrow 😄

self.output = _renderer.tap do |renderer|
renderer.layouts = layouts
renderer.payload = site_payload
end.run

This comment has been minimized.

@ayastreb

ayastreb Sep 9, 2016

Contributor

@parkr since we're not removing Convertible at least till Jekyll 4, maybe I should just rollback this change and just call do_layout as it was before?
Because now it just duplicates part of the Convertible.do_layout

@ayastreb

ayastreb Sep 9, 2016

Contributor

@parkr since we're not removing Convertible at least till Jekyll 4, maybe I should just rollback this change and just call do_layout as it was before?
Because now it just duplicates part of the Convertible.do_layout

This comment has been minimized.

@parkr

parkr Sep 13, 2016

Member

Yeah, I think that would work!

@parkr

parkr Sep 13, 2016

Member

Yeah, I think that would work!

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Sep 9, 2016

Contributor

Ok, I've done all the edits and rebased the branch, should be ready now 😄

Contributor

ayastreb commented Sep 9, 2016

Ok, I've done all the edits and rebased the branch, should be ready now 😄

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 19, 2016

Member

/cc: @jekyll/core for review

Member

pathawks commented Oct 19, 2016

/cc: @jekyll/core for review

@pathawks

💟👏🎉

joallard referenced this pull request Nov 2, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 10, 2016

Member

@parkr Is this good to merge?

Member

pathawks commented Dec 10, 2016

@parkr Is this good to merge?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 15, 2016

Member

@parkr Is this good to merge?

It looks like I have some unaddressed questions! I'd also love to see this brought up-to-date with master.

Member

parkr commented Dec 15, 2016

@parkr Is this good to merge?

It looks like I have some unaddressed questions! I'd also love to see this brought up-to-date with master.

parkr added a commit that referenced this pull request Mar 31, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

Merged in 1c33bd5 🎉 !

Member

parkr commented Mar 31, 2017

Merged in 1c33bd5 🎉 !

@parkr parkr closed this Mar 31, 2017

parkr added a commit that referenced this pull request Mar 31, 2017

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