Allow curly brackets and escaped quotes in Variables #170
Conversation
|
+1 for this |
|
addition: closing double brackets are allowed in variables. |
|
oh, the build failed. but i don't care because ruby 1.8.* is just supported until summer 2013 |
|
hey! could you clarify what the use case of this is for an end user? i currently don't understand why you would need quotes or curly braces in variables names. thanks! |
|
i'm writing a i18n filter for a project where i have to pass multiple (named) arguments (pluralization, maybe other variables) and my idea was to pass these variables as json-string. |
|
if there is a programmatic way of creating the translation values (i hope nobody should write JSON in the templates!) you could bypass the whole parsing issue by encoding the json with base46 like so: require "base64"
require "json"
encoded = Base64.encode64(JSON.dump({'count' => 10, 'custom_var' => 'hello world'})).chomp
# => "eyJjb3VudCI6MTAsImN1c3RvbV92YXIiOiJoZWxsbyB3b3JsZCJ9"
JSON.parse(Base64.decode64(encoded))
# => {"count"=>10, "custom_var"=>"hello world"} |
There was a problem hiding this comment.
Isn't this testing the same thing as the previous test? Why do we need the previous one?
|
Can we get a performance benchmark for this? |
|
I noticed |
|
@fw42 i will run one in the next days |
|
@emrox I agree. I tested closing an output segment with a single Liquid::Template.parse('hi {{ name }').render('name' => 'Joe')
# Liquid::SyntaxError: Variable '{{ name }' was not properly terminated with regexp: /\}\}/So, as I understand it, the invalid template is being parsed but is not being processed. I would say it's safe to remove |
|
VariableIncompleteEnd appears to be present to be forgiving for when a variable isn't closed properly. We would like liquid to be backwards compatible so that existing liquid templates don't break, so I don't think it should be removed. Regardless, it is outside the scope of this pull request. A pull request should only address a single issue. |
|
I would like to get this into 2.6, but we also want 2.6 to be (the last release which is) Ruby 1.8 compatible. Can you fix the broken 1.8 tests? |
|
Benchmarks with ruby 2.0.0-p195 @fw42 Original With Patch |
|
i have no problems running the tests on 1.8.7 $ rvm use 1.8.7
Using /home/stefan/.rvm/gems/ruby-1.8.7-p358
$ ruby -v
ruby 1.8.7 (2012-02-08 patchlevel 358) [x86_64-linux]
$ rake test
Couldn't load ruby-debug. gem install ruby-debug if you need it.
Run options:
# Running tests:
.....................................................................................................................................................................................................................................................................................................................
Finished tests in 0.078762s, 3923.2248 tests/s, 9789.0171 assertions/s.
309 tests, 771 assertions, 0 failures, 0 errors, 0 skipsi found no way to restart the test on travis ci without recommiting, so it will restarted when i implemented the new test mentioned in #213 |
|
|
Same with p358 for me |
|
I get similar numbers: Before: After: @boourns, big deal? |
ghost
assigned
|
@emrox, any progress (or interest) in fixing the broken tests? |
|
no progress atm, but still interested in fixing these tests. |
|
+1 for this |
|
Just a note to myself (and any one interested): the problem is in liquid.rb PartialTemplateParser when you remove the * on the groups, everything is fine, but one test where it is needed is failing. Unfortunately it's the test why i created this patch .. i'm still looking for a solution, thats just the state where i'm stuck atm. |
|
finaly i'm stuck. i can't see any easy way to fix this in 1.8.7 using regexp. for me it seems like a flaw in the regexp engine of 1.8.7. |
|
Ok, that's fine with me, thanks for the effort @emrox! |
|
@emrox: Can you rebase against master and see if tests pass? (We dropped 1.8 support and removed it from CI). |
|
I'm closing this, since there is no progress for almost a year and it doesn't pass tests anymore on current master (just tried it). Please reopen if you are still interested in getting this merged. |
|
I am still interested in this one. I can finalize this request if there are concerns. |
|
No concerns, if you can make a PR with green tests, we can run a performance benchmark again. |
|
This is the failing tests that I get (on Ruby 2.1.2): |
|
@fw42 sure thing I need to fix it, but confirm that you gonna merge it after that. |
|
I can't promise you that, but I can confirm that we will take another look, do some performance benchmarks, and discuss it again. |
|
ok, do you want me to do new PR or you will reopen old one? |
|
open a new one please |
|
i fixed it, but need to create a new branch. |
|
@emrox i'm back in hamburg next week, btw |
|
:-( |
|
should i create a new request? @phoet \o/ |
|
The test that you removed in that commit was there for a reason... It's a regression test for something that caused troubles in Shopify, see #213 (comment). |
|
Some more info: @dylanahsmith tried to fix a similar issue in #325, but we had to revert it because it broke certain templates (the relevant part of which was captured in that new regression test). So please don't remove it and make sure it still passes :-) |
|
@fw42 you are right, i fixed it emrox@c5015dd benchmark before (original master): benchmark after: |
|
Hm that's kind of a big hit :-/ Can you open a new PR with all that and I will ping our performance team to discuss |
Depending on the context, the value should be 'user'
8 participants
{{ '{'message': 'this should work now', 'foo': {'bar':'rab'}}' }}