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

Upgrade liquid #1982

Merged
merged 3 commits into from
Apr 26, 2017
Merged

Upgrade liquid #1982

merged 3 commits into from
Apr 26, 2017

Conversation

dsander
Copy link
Collaborator

@dsander dsander commented Apr 24, 2017

Fixes #1981

Copy link

@RussMorg-SMSDev RussMorg-SMSDev left a comment

Choose a reason for hiding this comment

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

Thumbs up, guys! 😄

@@ -0,0 +1,21 @@
module Agents
class SleepAgent < Agent
Copy link
Member

Choose a reason for hiding this comment

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

@dsander did you mean to include this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Woups, no 😄

@cantino
Copy link
Member

cantino commented Apr 25, 2017

Makes sense to me, although I haven't dug into the Liquid BlockBody class. Anything else we should test?

@dsander
Copy link
Collaborator Author

dsander commented Apr 25, 2017

@cantino The test coverage looks pretty good for the LiquidInterpolatable module so we should be good.

@knu Could you give it a second look? I more or less followed the changes the liquid team did to their own tags but noticed the node_list method was not called before and isn't now. Do you remember why it was added?

@cantino
Copy link
Member

cantino commented Apr 25, 2017

👍

@knu
Copy link
Member

knu commented Apr 26, 2017

@dsander The nodelist method is defined in the superclass and I just overrode it in the subclass. The method is currently only called when emitting warnings, so it may not be covered by our specs.

Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -386,21 +386,27 @@ def initialize(tag_name, markup, tokens)
else
raise Liquid::SyntaxError, 'Syntax Error in regex_replace tag - Valid syntax: regex_replace pattern in'
end
@nodelist = @in_block = []
@with_block = nil
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better not to remove this initialization, because it was there to avoid a runtime warning about reference to an uninitialized instance variable when $VERBOSE is turned on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, I did not know about those warnings.

@dsander
Copy link
Collaborator Author

dsander commented Apr 26, 2017

@knu Thanks for the clarification and review!

@dsander dsander merged commit e1f702a into master Apr 26, 2017
@dsander dsander deleted the upgrade-liquid branch April 26, 2017 20:09
@cantino
Copy link
Member

cantino commented Apr 26, 2017

Thanks @dsander!

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.

None yet

4 participants