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.

Sign up for GitHub

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

Jump to bottom

Fold private methods into the :render method as local variables #327

Merged
merged 3 commits into from Dec 16, 2021
Merged

Fold private methods into the :render method as local variables #327

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f0dd6d
Memoize feed tag markup string
ashmaroli Oct 4, 2020
6862469
Replace private methods with local variables
ashmaroli Oct 5, 2020
9ffd182
Fold all private methods into method `:render`
ashmaroli Oct 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 9 additions & 6 deletions lib/jekyll-feed/meta-tag.rb
View file
Open in desktop
Expand Up @@ -6,16 +6,19 @@ class MetaTag < Liquid::Tag
include Jekyll::Filters::URLFilters

def render(context)
@context = context
attrs = attributes.map do |k, v|
v = v.to_s unless v.respond_to?(:encode)
%(#{k}=#{v.encode(:xml => :attr)})
end
"<link #{attrs.join(" ")} />"
@context ||= context
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since context could theoretically change (and therefore the code should reflect this), I'd feel more comfortable with an approach using the context's hash (the number Ruby assigns to each unique object):

def render(context)
  link_tag(context)
end

def link_tag(context)
  @link_tags[context.hash] ||= "<link #{xml_encoded_attributes(context).join(" ")} />"
end

etc.

In this model, we always pass context down through the methods, making them pure functions, instead of relying on @context being set (and therefore acting more like Object methods relying on state). What do you think about an approach like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the source-code on master, the only place @context is ever used in this Liquid tag instance is to get the site config:

jekyll-feed/lib/jekyll-feed/meta-tag.rb

Lines 19 to 21 in 653999c

def config
@config ||= @context.registers[:site].config
end

Since the site config shouldn't be changing once site.render phase starts, memoizing with a hash-store therefore seems wasteful to me...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@context is required to be set for Jekyll::Filters::URLFilters to work.
So the suggested model is not applicable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the site config shouldn't be changing once site.render phase starts, memoizing with a hash-store therefore seems wasteful to me...

The code as it exists today has a render method that takes in a context. That means that render should respond to changes in context. From a strict read of just the method signatures, one would expect that inputting new information into context and passing it to render would yield a different result. I think the only way to fix this that can use an object-method approach is to do MetaTag.new(context).render and update everything accordingly. The problem stems from the programmer's expectation that f(input) should always take input into account rather than just the input to the first call to f(). This hybrid it's-a-function-but-we-treat-it-like-an-object relies on implicit understanding of the implementation, which could change. The function signatures should reflect this and move fully into an object approach, or use a pure function approach instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay @parkr.

To avoid further bike-shedding, I've decided to not memoize at all..
(since this tag is typically used once per page and hence the context.hash is never constant)

I have refactored the whole implementation to not call private methods at all.

Is the current diff acceptable to you?

memoized_result
end

private

def memoized_result
@memoized_result ||= begin
attrs = attributes.map { |k, v| "#{k}=#{v.to_s.encode(:xml => :attr)}" }
"<link #{attrs.join(" ")} />"
end
end

def config
@config ||= @context.registers[:site].config
end
Expand Down