Fold private methods into the :render method as local variables

[ashmaroli]

Since the meta tag contents doesn't depend on page-specific metadata, the resulting markup is essentially the same for all pages / documents in a site:

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

Lines 23 to 38 in 653999c

	def attributes
	{
	:type => "application/atom+xml",
	:rel => "alternate",
	:href => absolute_url(path),
	:title => title,
	}.keep_if { |_, v| v }
	end
	def path
	config.dig("feed", "path") || "feed.xml"
	end
	def title
	config["title"] || config["name"]
	end

Therefore in theory, the result can be safely memoized to avoid generating arrays for every page / document..

[ashmaroli]

/cc @parkr to verify that this doesn't interfere with the jekyll-github-metadata plugin..

[parkr]

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?

[ashmaroli]

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

[ashmaroli]

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

[parkr]

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.

[ashmaroli]

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?

[hugoglezcon]

Gracias pero necesario ver el conte

[parkr]

@jekyllbot: merge +bug