Allow curly brackets and escaped quotes in Variables (2) #392
Conversation
|
@camilo, @dylanahsmith, thoughts? |
There was a problem hiding this comment.
Why do you have to explicitly specify the error_mode here?
There was a problem hiding this comment.
without i get an liquid parser error (i think it's because the liquid parser doesn't like the ' ?)
There was a problem hiding this comment.
well, given that the point of this PR is to fix a parser bug, forcing a test to not run with one of the two parsers seems like the wrong approach to fixing the test :-P
There was a problem hiding this comment.
when i try this on the original master, i also get an error:
diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb
index add77c3..fadb642 100644
--- a/test/integration/parsing_quirks_test.rb
+++ b/test/integration/parsing_quirks_test.rb
@@ -91,4 +91,12 @@ class ParsingQuirksTest < Test::Unit::TestCase
end
end
+ def test_strings_with_escaped_quotes
+ text = 'test \\" escaping'
+
+ assert_nothing_raised do
+ Template.parse("{{ \"#{text}\" }}")
+ end
+ end
+
end # ParsingQuirksTestwith error_mode: :lax everything is fine
should i open a new issue for that?
There was a problem hiding this comment.
I'm confused now. I thought the fact that this doesn't work (escaped quotes) is what this PR is supposed to fix. Of course it fails on master :-) My point is that it seems like you only fixed it for the lax parser, but not the strict parser.
There was a problem hiding this comment.
damn, you are right. i should read my own headlines :(
i was fixed on the curly brackets
|
Before you put too much effort in this, we should discuss if the performance penalty is worth it. |
|
Hmmm...can I see profile output for both master and this change? Both cpu and object please... (see #381). Perf penalty seems high to me. |
|
I think the escaping is actually not the important part here, you can avoid that by using different quotes. You could use Liquid code in your template like this: and not need any escaping. But the curly brackets is what breaks this. |
|
@fw42 for testing i removed the quote part benchmark original master: benchmark patched with just the curly bracket part: |
|
+1 on @jasonhl the penalty seems high object and cpu profiles would be good maybe is something easy to fix. |
|
Original Profile: Change Profile (without the quote escaping patch): Change profile including the (currently just working in lax mode) escape patch: |
|
the patch is just a regex change, so all work belongs to the ruby regex engine. i think optimizing the regex should be hard work. |
|
Thank you for the benchmarks/profiles. Looks like there's no object allocation penalty but there is a ~10% penalty on parsing. That's pretty big. The problem is that the speed of liquid (lax) parsing is almost entirely dependent on those regexes. Changing them has a disproportionate effect on the speed of the entire process. So, unless the negative performance effect can be reduced, I'm going to vote 👎 on this one. Too high a penalty for what is effectively a corner case. |
|
atm i can't see a way to make the regex faster. sure, i can strip the quote-escaping part, but the pure regex (without the other liquid stuff around) is still 2 times slower then the current liquid regex. Result: |
|
Given the performance penalty of this, I'm afraid we will have to close this for now. Sorry! Thanks so much for taking the time to pull this together. I would love to get rid of all those shitty little parser quirks, but at the end of the day, they are "just" edge cases and fixing them all would slow us down significantly :-( |
4 participants
The sequel to #170
With this patch this should work now:
the main idea was to use it for a i18n filter accepting json encoded strings as argument
the patch should also fix #251
Benchmark result of the original current liquid master:
Benchmark result with this patch: