Avoid direct coupling to BlockBody instances for liquid-c replacement#1289
Merged
Conversation
This was referenced Sep 8, 2020
This way liquid-c can return a body of a different class that wraps a C implementation.
So they can be used from a Liquid::C::BlockBody
This way liquid-c can more cleanly use a Liquid::C::BlockBody object for the block body by overriding Liquid::Document#new_body.
dylanahsmith
force-pushed
the
refactor-for-c-block-body
branch
from
e146906 to
dfbbf87
Compare
Thibaut
reviewed
Sep 9, 2020
|
|
||
| def new_body | ||
| Liquid::BlockBody.new | ||
| end |
Contributor
There was a problem hiding this comment.
This repeats the implementation in Liquid::Block. If the two are always intended to do the same thing / be overridden together, how about we expose a single implementation somewhere higher up? (for example, Liquid.new_block_body) This way Liquid C only needs to override one method.
Contributor
Author
There was a problem hiding this comment.
Pushed a commit that extracted the implementation to override into ParseContext#new_block_body. I'll update the liquid-c PR to override that.
Thibaut
approved these changes
Sep 9, 2020
pushrax
approved these changes
Sep 11, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
We want to use a C data structure for the block body in liquid-c (Shopify/liquid-c#58) that will be exposed to ruby as Liquid::C::BlockBody, but currently Liquid::Document is directly coupled to Liquid::BlockBody. There are also several places where
BlockBody.newis used directly, which doesn't provide a clean way to override in liquid-c to useLiquid::C::BlockBody. instead.Solution
The first commit adds Liquid::Block#new_body that just returns
BlockBody.newso that it can be overridden in liquid-c to returnLiquid::C::BlockBody.new.The second commit exposes some Liquid::BlockBody instance methods as class methods (raise_missing_tag_terminator, raise_missing_variable_terminator, render_node), since they don't depend on
selfand can be re-used for simplicity in liquid-c.The third commit changes Liquid::Document to not inherit from Liquid::BlockBody, but instead to store a Liquid::BlockBody. It also creates the block body using a
#new_bodymethod that can be overridden by liquid-c.