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

Fix Liquid/C BlockBody loading issue #874

Closed
wants to merge 1 commit into from

Conversation

fw42
Copy link
Contributor

@fw42 fw42 commented Mar 20, 2017

Fixes Shopify/liquid-c#32.

Note that this PR is against the 3-0-stable branch, not against master. The problem doesn't exist on the master branch.

cc @wjdp

Copy link
Contributor

@dylanahsmith dylanahsmith left a comment

Choose a reason for hiding this comment

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

Looks like commit 4b1835e didn't properly revert #458, since it left in block_body.rb even though it is unused. Instead of requiring dead code we should be removing it.

liquid-c isn't going to work properly by monkey patching dead code, so we should instead update the version constraint to require liquid versions ~> 4.0

@@ -57,6 +57,7 @@ module Liquid
require 'liquid/parser_switching'
require 'liquid/tag'
require 'liquid/block'
require 'liquid/block_body'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is block_body.rb even in this branch? Liquid::BlockBody isn't used by any of the code in this branch.

@fw42
Copy link
Contributor Author

fw42 commented Mar 21, 2017

Good point, I totally missed that

@fw42
Copy link
Contributor Author

fw42 commented Mar 21, 2017

we should instead update the version constraint to require liquid versions ~> 4.0

You mean Liquid 3 should depend on Liquid-C 4?

Shouldn't we rather remove the BlockBody dependency from Liquid-C version 3?

@dylanahsmith
Copy link
Contributor

Oh, I thought you meant the problem was with using the latest version of liquid-c with an old version of liquid. Ya, it looks like we meant to have the liquid-c 3.x be compatible with liquid 3.x and it looks like that isn't the case

@fw42 fw42 closed this Sep 4, 2019
@fw42 fw42 deleted the fix-liquid-c-loading-issue branch September 4, 2019 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants