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.

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

Alias Drop#invoke_drop to Drop#[] #6338

Merged
merged 7 commits into from Sep 6, 2017
Merged

Alias Drop#invoke_drop to Drop#[] #6338

merged 7 commits into from Sep 6, 2017

Conversation

@benbalter
Copy link
Contributor

benbalter commented Aug 30, 2017

@DirtyF discovered an interesting behavior via jekyll/github-metadata#108 (comment), which I had attempted to resolve (unsuccessfully) via jekyll/github-metadata#110.

In the particular instance, Jekyll::Utils.deep_merge_hashes(drop, hash) was returning a drop as expected, but without the hash values overriding the drop values. After some digging, I discovered the merge logic was correct (and calling [key]=value, which gets stored in @mutations), but that when a drop method was called, it was returning the drop's value, not the mutation.

If a drop is mutable, #[] prefers the user-set value over the default (a Jekyll construct).

#invoke_drop(key) which Liquid uses, however, doesn't understand the concept of mutations as it's using Liquid::Drop's native implementation. This leads to scenarios where drop.key, drop.invoke_drop('key') and drop["key"] return different values.

Normally, Drop#[] is aliased to Drop#invoke_drop ensuring the values remain in sync. Since they both implement the same core logic public_send key if self.class.invokable? key, by aliasing #invoke_drop to #[], both will respect mutability and return the same value regardless of drop.invoke_drop('key') or drop['key] is called (and allowing Jekyll::Utils.deep_merge_hashes to perform the expected operation).

Given the following test case:

require 'jekyll'

class TestDrop < Jekyll::Drops::Drop
  mutable true

  def fallback_data
    @fallback_data ||= {}
  end

  def value
    "value from drop method"
  end

  alias_method :invoke_drop, :[]
end

obj = { "value" => "value from drop object" }
drop = TestDrop.new(obj)

puts "\ndrop.to_h after construction:"
puts drop.to_h

hash = { "value" => "value from hash" }
drop = drop.merge(hash)

puts "\ndrop.to_h after merge:"
puts drop.to_h

puts "\ndrop.invoke_drop('value') after merge:"
puts drop.invoke_drop('value')

puts "\ndrop['value'] after merge:"
puts drop['value']

Output before change:

drop.to_h after construction:
{"value"=>"value from drop method"}

drop.to_h after merge:
{"value"=>"value from hash"}

drop.invoke_drop('value') after merge:
value from drop method

drop['value'] after merge:
value from hash

Output after change:

drop.to_h after construction:
{"value"=>"value from drop method"}

drop.to_h after merge:
{"value"=>"value from hash"}

drop.invoke_drop('value') after merge:
value from hash

drop['value'] after merge:
value from hash

The one gotcha, is with this implementation is we lose the liquid_method_missing logic, which we could encorporate into #[] if we want, but isn't strictly necessary in our implementation since we never raise for missing methods.

If a drop is mutable, #[] preferes the user-set value over the default (a Jekyll construct).

This leads to scenarios where drop.value and drop["value"] return different values.

By aliasing #invoke_drop to #[], both will respect mutability and return the same value.
@DirtyF

This comment has been minimized.

Copy link
Member

DirtyF commented Aug 30, 2017

@benbalter Great explanation, I'm a big fan of how you document issues to allow others to learn. 👏

benbalter added a commit to jekyll/github-metadata that referenced this pull request Aug 30, 2017
Copy link
Member

parkr left a comment

Love it, and totally down to include this change in 3.6 and possibly backport. In order to ensure we don't lose this behaviour in the future, could you add a test to test/test_utils.rb?

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Aug 30, 2017

Whoops – test/test_drop.rb would be a more appropriate place to add a test for this.

@benbalter

This comment has been minimized.

Copy link
Contributor Author

benbalter commented Aug 30, 2017

could you add a test

Implemented via a948998.

benbalter added 2 commits Aug 30, 2017

def fallback_data
@fallback_data ||= {}
end

This comment has been minimized.

Copy link
@benbalter

benbalter Aug 30, 2017

Author Contributor

The base drop class (in #[], #keys and #key?) assumes fallback data to exist and return a hash. Stubbed out the default value so that child classes don't each have to implement it if they don't have fallback data (including our test fixture).

This comment has been minimized.

Copy link
@parkr

parkr Aug 30, 2017

Member

I intentionally left this method out so all drops had to implement this – it was part of the design of Jekyll Drops, that some fallback data had to be provided by the developer of a drop. I'm not sure I like that going away. :/

This comment has been minimized.

Copy link
@benbalter

benbalter Aug 30, 2017

Author Contributor

it was part of the design of Jekyll Drops, that some fallback data had to be provided by the developer of a drop

Admittedly out of scope, so glad to remove from this PR. Curious if that rationale still holds true when Jekyll internally often implements it as an empty hash?

This comment has been minimized.

Copy link
@benbalter

benbalter Sep 5, 2017

Author Contributor

Removed via 847ed7f

benbalter added 3 commits Sep 5, 2017
@benbalter

This comment has been minimized.

Copy link
Contributor Author

benbalter commented Sep 5, 2017

Fixing this one problem revealed another blocker to mutability taking precedence over methods.

Take a look at 3776c44 (failing test) and c998c59 (fix).

In short, key? called mutable not mutable? meaning calls to fetch or Liquid's own use of key? at render time would call class.mutable without any arguments. Doing so would (A) always return false, but more importantly (B) set @is_mutable to false, meaning fetching a mutable key would zero out all mutability (even if accessed using another method like #[].

This now PR simply changes self.class.mutable to self.class.mutable? which is the getter, not the setter.

@parkr
parkr approved these changes Sep 5, 2017
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Sep 5, 2017

Sounds like we should backport this to 3.5.x once it ships.

👍 to merge once CI is passing.

benbalter added a commit to jekyll/github-metadata that referenced this pull request Sep 5, 2017
@benbalter

This comment has been minimized.

Copy link
Contributor Author

benbalter commented Sep 6, 2017

@jekyllbot: merge +minor.

@jekyllbot jekyllbot merged commit 1637f29 into master Sep 6, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jekyllbot jekyllbot deleted the mutable-drop-methods branch Sep 6, 2017
jekyllbot added a commit that referenced this pull request Sep 6, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.