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

Upgrade to Liquid v4 #4362

Merged
merged 15 commits into from Jan 27, 2017

Conversation

Projects
None yet
@parkr
Member

parkr commented Jan 16, 2016

RC1 for now.

@fw42 @dylanahsmith, know what's going on here? .parse isn't working like before.

Error:
TestTags#test_: highlight tag in unsafe mode should recognize the hl_linenos option and its value. :
NoMethodError: undefined method `line_number' for {}:Hash
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/liquid-4.0.0.rc1/lib/liquid/tag.rb:21:in `initialize'
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/liquid-4.0.0.rc1/lib/liquid/block.rb:4:in `initialize'
    /Users/parkr/jekyll/jekyll/lib/jekyll/tags/highlight.rb:14:in `initialize'
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/liquid-4.0.0.rc1/lib/liquid/tag.rb:9:in `new'
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/liquid-4.0.0.rc1/lib/liquid/tag.rb:9:in `parse'
    test/test_tags.rb:46:in `highlight_block_with_opts'
    test/test_tags.rb:92:in `block (2 levels) in <class:TestTags>'
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `instance_exec'
    /Users/parkr/.rbenv/versions/2.3.0/lib/ruby/gems/2.3.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `block in create_test_from_should_hash'

@parkr parkr added the Enhancement label Jan 16, 2016

@parkr parkr added this to the 3.2 milestone Jan 16, 2016

@dylanahsmith

This comment has been minimized.

Show comment
Hide comment
@dylanahsmith

dylanahsmith Jan 16, 2016

Jekyll::Tags::HighlightBlock.parse('highlight', options_string, ["test", "{% endhighlight %}", "\n"], {})
passes in a list of tokens into a Tag, but we now use a tokenizer which also keeps track of the current line number during parsing.

This could be worked around by using Liquid::Tokenizer.new("test{% endhighlight %}\n"), but the tests would still be brittle to internal liquid changes, so it is recommended to parse a complete template using Liquid::Template rather than constructing individual tags with mocked out data.

dylanahsmith commented Jan 16, 2016

Jekyll::Tags::HighlightBlock.parse('highlight', options_string, ["test", "{% endhighlight %}", "\n"], {})
passes in a list of tokens into a Tag, but we now use a tokenizer which also keeps track of the current line number during parsing.

This could be worked around by using Liquid::Tokenizer.new("test{% endhighlight %}\n"), but the tests would still be brittle to internal liquid changes, so it is recommended to parse a complete template using Liquid::Template rather than constructing individual tags with mocked out data.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jan 16, 2016

Contributor

Would this affect normal everyday plugins @dylanahsmith ?

Contributor

envygeeks commented Jan 16, 2016

Would this affect normal everyday plugins @dylanahsmith ?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 16, 2016

Member

it is recommended to parse a complete template using Liquid::Template rather than constructing individual tags with mocked out data.

👍

Would this affect normal everyday plugins

This looks like it's only in our tests where this is going to be an issue. Most plugins don't use this Tag.parse method directly, I think only our tests do that.

Member

parkr commented Jan 16, 2016

it is recommended to parse a complete template using Liquid::Template rather than constructing individual tags with mocked out data.

👍

Would this affect normal everyday plugins

This looks like it's only in our tests where this is going to be an issue. Most plugins don't use this Tag.parse method directly, I think only our tests do that.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 23, 2016

Member

@dylanahsmith Our tests depend on having access to the instance created. I gave it a shot with Liquid::Template.parse but gave up after about 30 minutes of looking through the Liquid code.

It looks like v4 works (CI works fine for me locally). Would you consider releasing a new version soon and we can upgrade?

Member

parkr commented Mar 23, 2016

@dylanahsmith Our tests depend on having access to the instance created. I gave it a shot with Liquid::Template.parse but gave up after about 30 minutes of looking through the Liquid code.

It looks like v4 works (CI works fine for me locally). Would you consider releasing a new version soon and we can upgrade?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 30, 2016

Member

@dylanahsmith @fw42 @pushrax Hi! Would it be possible to reconsider a 4.0 release? Especially keen to get the whitespace change out to our users. Thanks!

Member

parkr commented Aug 30, 2016

@dylanahsmith @fw42 @pushrax Hi! Would it be possible to reconsider a 4.0 release? Especially keen to get the whitespace change out to our users. Thanks!

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Aug 30, 2016

Contributor

I'm ok with releasing a new version of Liquid. As far as I know, there isn't any pending non-backwards compatible changes that we want to get in before a new release. @pushrax @dylanahsmith, thoughts?

Contributor

fw42 commented Aug 30, 2016

I'm ok with releasing a new version of Liquid. As far as I know, there isn't any pending non-backwards compatible changes that we want to get in before a new release. @pushrax @dylanahsmith, thoughts?

@dylanahsmith

This comment has been minimized.

Show comment
Hide comment
@dylanahsmith

dylanahsmith Aug 30, 2016

version ⬆️ 👍

dylanahsmith commented Aug 30, 2016

version ⬆️ 👍

@pushrax

This comment has been minimized.

Show comment
Hide comment
@pushrax

pushrax Sep 1, 2016

Also 🆗 from me

pushrax commented Sep 1, 2016

Also 🆗 from me

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Sep 13, 2016

Contributor

Yea I just released 4.0rc versions of both Liquid and Liquid-C. @parkr, can you see if everything works on your end? If so, I can release a real non-rc release.

Contributor

fw42 commented Sep 13, 2016

Yea I just released 4.0rc versions of both Liquid and Liquid-C. @parkr, can you see if everything works on your end? If so, I can release a real non-rc release.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 16, 2016

Member

Interesting –– all of our where_exp tests fail.

Member

parkr commented Sep 16, 2016

Interesting –– all of our where_exp tests fail.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 19, 2016

Member

Looks like things are working here. I'd be interested in getting this into a v3.4, though I have concerns about what incompatibilities for plugin authors.

Member

parkr commented Oct 19, 2016

Looks like things are working here. I'd be interested in getting this into a v3.4, though I have concerns about what incompatibilities for plugin authors.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Oct 19, 2016

Member

@parkr I think the only public API thing we changed was to add Jekyll::Drop::SiteDrop#key?. Do you know of any liquid-related plugins so we could check them out before?

Also if you think we're ok, we could give @fw42 a go to release 4.0 non-rc 🎆

Member

Crunch09 commented Oct 19, 2016

@parkr I think the only public API thing we changed was to add Jekyll::Drop::SiteDrop#key?. Do you know of any liquid-related plugins so we could check them out before?

Also if you think we're ok, we could give @fw42 a go to release 4.0 non-rc 🎆

@DirtyF DirtyF referenced this pull request Nov 11, 2016

Closed

Jekyll 4.0 Wishlist #5307

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 29, 2016

Member

@fw42 If this CI passes, then feel free to release v4.0 of Liquid. ❤️

Member

parkr commented Nov 29, 2016

@fw42 If this CI passes, then feel free to release v4.0 of Liquid. ❤️

@parkr parkr referenced this pull request Nov 29, 2016

Closed

The roadmap for 4.0 release #818

@parkr parkr self-assigned this Nov 29, 2016

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Dec 5, 2016

Contributor

We have a few last minute breaking changes that we want to get in, but I will try to release v4 this week if possible.

Contributor

fw42 commented Dec 5, 2016

We have a few last minute breaking changes that we want to get in, but I will try to release v4 this week if possible.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 7, 2016

Member

@fw42 More Breaking changes may mean we can't upgrade 😦 Would it be possible to release a 5.0 with those changes? We may be stuck on 3.x if we can't guarantee compatibility at the high level for plugins. Or, perhaps we can run our test suite against the individual pull requests?

Member

parkr commented Dec 7, 2016

@fw42 More Breaking changes may mean we can't upgrade 😦 Would it be possible to release a 5.0 with those changes? We may be stuck on 3.x if we can't guarantee compatibility at the high level for plugins. Or, perhaps we can run our test suite against the individual pull requests?

@dylanahsmith

This comment has been minimized.

Show comment
Hide comment
@dylanahsmith

dylanahsmith Dec 7, 2016

@parkr Shopify/liquid#835 is the change that I would like to get into liquid v4, since it provides a more secure default for rendering errors, so that sensitive information doesn't get leaked through the error message. I tried running the tests on this jekyll branch against that liquid branch and everything passed. It looks like jekyll uses Liquid::Template#render!, which isn't affected by that pull request, since it will continue to re-raise the exception as it did before.

dylanahsmith commented Dec 7, 2016

@parkr Shopify/liquid#835 is the change that I would like to get into liquid v4, since it provides a more secure default for rendering errors, so that sensitive information doesn't get leaked through the error message. I tried running the tests on this jekyll branch against that liquid branch and everything passed. It looks like jekyll uses Liquid::Template#render!, which isn't affected by that pull request, since it will continue to re-raise the exception as it did before.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 7, 2016

Member

@dylanahsmith Perfect, thank you!

Member

parkr commented Dec 7, 2016

@dylanahsmith Perfect, thank you!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 7, 2016

Contributor

Yay for Liquid 4.

Contributor

envygeeks commented Dec 7, 2016

Yay for Liquid 4.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 15, 2016

Member

Liquid 4 is now officially here 🎉
ad00998

Member

ashmaroli commented Dec 15, 2016

Liquid 4 is now officially here 🎉
ad00998

@parkr parkr requested a review from pathawks Dec 15, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Dec 15, 2016

Member

I don't think I'll have time to put Liquid 4 through it's paces for another week or so, but I am super excited for some of the new features it brings!

Member

pathawks commented Dec 15, 2016

I don't think I'll have time to put Liquid 4 through it's paces for another week or so, but I am super excited for some of the new features it brings!

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Dec 16, 2016

Member

I wonder why tests suddenly fail when they had passed with v4.0.0.rc3

Member

ashmaroli commented Dec 16, 2016

I wonder why tests suddenly fail when they had passed with v4.0.0.rc3

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Dec 16, 2016

Contributor

Test failures look related to Shopify/liquid#835 or Shopify/liquid#837.

Contributor

fw42 commented Dec 16, 2016

Test failures look related to Shopify/liquid#835 or Shopify/liquid#837.

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Dec 16, 2016

Contributor

Probably the latter since @dylanahsmith said that he tested the former with this branch.

Contributor

fw42 commented Dec 16, 2016

Probably the latter since @dylanahsmith said that he tested the former with this branch.

@pushrax

This comment has been minimized.

Show comment
Hide comment
@pushrax

pushrax Dec 16, 2016

Oh god, working around Liquid's lack of expressions with strings 😭

We (@gmalette, @sgnr) have been talking a bit about how to add proper expression support to Liquid. I think we can do it in 2017.

pushrax commented Dec 16, 2016

Oh god, working around Liquid's lack of expressions with strings 😭

We (@gmalette, @sgnr) have been talking a bit about how to add proper expression support to Liquid. I think we can do it in 2017.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 16, 2016

Member

Having a Liquid::Variable.parse class method would make it more consistent with Liquid::Template and tags classes

@dylanahsmith I like that idea. I think we're fairly comfortable breaking the rules a bit for now since there aren't tons of releases of Liquid and y'all are very kind about helping us test our code when you do want to make updates. Having Liquid::Variable.parse that offers a stable external API would be preferable, of course, but we're aware that our use of Liquid is not really your primary development concern.

Is there a way for us to make our current usage line up better with how it should work, e.g. by passing the proper Liquid::ParseContext from the parent template?

Member

parkr commented Dec 16, 2016

Having a Liquid::Variable.parse class method would make it more consistent with Liquid::Template and tags classes

@dylanahsmith I like that idea. I think we're fairly comfortable breaking the rules a bit for now since there aren't tons of releases of Liquid and y'all are very kind about helping us test our code when you do want to make updates. Having Liquid::Variable.parse that offers a stable external API would be preferable, of course, but we're aware that our use of Liquid is not really your primary development concern.

Is there a way for us to make our current usage line up better with how it should work, e.g. by passing the proper Liquid::ParseContext from the parent template?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 16, 2016

Member

Well, I have certainly sullied up this branch. Let's see if I can fix that git history...

Member

parkr commented Dec 16, 2016

Well, I have certainly sullied up this branch. Let's see if I can fix that git history...

parkr and others added some commits Jan 16, 2016

@pathawks

❤️

Doesn't break any jekyll/* plugins 👍

@pathawks pathawks referenced this pull request Dec 22, 2016

Merged

[liquid 4] Utils #5679

pathawks and others added some commits Dec 22, 2016

@DirtyF DirtyF referenced this pull request Jan 10, 2017

Closed

Liquid 4.0.0 #5749

@JamesDrummond

This comment has been minimized.

Show comment
Hide comment
@JamesDrummond

JamesDrummond Jan 10, 2017

When will this PR be merged? Patiently waiting for liquid 4.0 features ;) . Also I was trying to find where you list when the next release and ship date is planned for.

JamesDrummond commented Jan 10, 2017

When will this PR be merged? Patiently waiting for liquid 4.0 features ;) . Also I was trying to find where you list when the next release and ship date is planned for.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 10, 2017

Member

I think we'll try to ship a 3.4 this week, with a 3.5 a bit after that.

Member

parkr commented Jan 10, 2017

I think we'll try to ship a 3.4 this week, with a 3.5 a bit after that.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 16, 2017

Member

Fixes #5772.

Member

parkr commented Jan 16, 2017

Fixes #5772.

@parkr parkr modified the milestones: 3.4, 3.5 Jan 16, 2017

@keslert

This comment has been minimized.

Show comment
Hide comment
@keslert

keslert Jan 24, 2017

Any update on this? Waiting not quite as patiently as @JamesDrummond ;).

keslert commented Jan 24, 2017

Any update on this? Waiting not quite as patiently as @JamesDrummond ;).

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Jan 24, 2017

Member

@keslert It is planned to be released in jekyll 3.5. Jekyll 3.4 should be out this week, so it probably will be merged in afterwards.

Member

Crunch09 commented Jan 24, 2017

@keslert It is planned to be released in jekyll 3.5. Jekyll 3.4 should be out this week, so it probably will be merged in afterwards.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 27, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Jan 27, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 2cf685f into master Jan 27, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot deleted the liquid-4 branch Jan 27, 2017

jekyllbot added a commit that referenced this pull request Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment