Does not catch extra brackets on the RHS

[DexiongYung]

For example "{{blah}}}" will not return an error, but it does for extra LHS brackets which seems odd.

[DexiongYung]

I work for a very large tech company(Not sure if I'm allowed to say on this public domain) and we've been using this gem for one of our major applications. There are a couple issues that might want to be addressed to make this more robust. I posted before about how missing LHS bracket such as "{}}" is not caught and you guys mentioned that was intentional, perhaps with "strict" on it does catch this. Here is the regex I've been using to catch that case: /(?:^|\s){\w*}}(?:\s|$)/

Also in cases where an improper keyword is used, it might be better to throw and error rather than just replace it with blank. I've handled this as such placeHolders = text.scan(/(?:^|\s){{\w*}}(?:\s|$)/) keywords = array of keywords placeHolders.each do |p| p = p.tr("{{}}","").tr(" ","") if !keywords.include? p errors.add(whatever error) return end end

Then for the above issue I mentioned(Too many RHS brackets not being caught) I've been using this regex /(?:^|\s){{\w*}}}+(?:\s|$)/ Because since too many LHS brackets is caught I don't have to worry about that case as much.

Just some suggestions for this gem to be used in a professional context(We're currently using it for templating emails from one company to another). Cheers!

[pushrax]

The most strict configuration looks like this:

irb(main):008:0> Liquid::Template.parse("{{ doesntexist }}", error_mode: :strict).render!({}, { strict_variables: true, strict_filters: true })
Liquid::UndefinedVariable: Liquid error: undefined variable doesntexist

See README.md for more on these options.

As you alluded to, extra closing braces are valid syntax. If this is a common mistake you're seeing in templates, adding an optional warning could be a PR we'd accept.

[DexiongYung]

I think it might also be a good idea to have the option to just get the last bit of the error message. For example instead of e.message = "Liquid::UndefinedVariable: Liquid error: undefined variable doesntexist" you could have something like e.problem = "undefined variable doesn'texist" for companies who want to have their own error handling, because for our product we needed user friendly messages that are clear, but uses part of the error that your gem produces.

[DexiongYung]

If you're okay with me adding another mode like "extra strict" that doesn't allow extra closing brackets and shows the last bit of the error message as mentioned above I could contribute to this gem(hopefully during work). I might need some assistance getting up to speed on the infrastructure though(Ruby is not my primary language).

[pushrax]

irb(main):019:0> begin
irb(main):020:1* Liquid::Template.parse("{{ doesntexist }}", error_mode: :strict).render!({}, { strict_variables: true, strict_filters: true })
irb(main):021:1> rescue => e
irb(main):022:1> puts e.message
irb(main):023:1> end
Liquid error: undefined variable doesntexist

# or

irb(main):024:0> tpl = Liquid::Template.parse("{{ doesntexist }}", error_mode: :strict)
irb(main):025:0> tpl.render({}, { strict_variables: true, strict_filters: true })
=> ""
irb(main):026:0> puts tpl.errors
Liquid error: undefined variable doesntexist
irb(main):027:0> puts tpl.errors.first.message
Liquid error: undefined variable doesntexist

This is the level of error we expose to merchants in Shopify. We need to tell them if a particular error is from Liquid or something else.

I was thinking the extra warnings could come in the style of lint warnings, rather than being a new mode it would be an option like lint: true. As mentioned, {{ x }}} is not a syntax error and wouldn't really make sense to be a syntax error.

[DexiongYung]

I'm not suggesting changing the error returned just an option to get the error minus the liquid string available. Hopefully I'm not disclosing more information than I should, but my company is using this gem for an administrator in our company to send a multiple emails to multiple company CEOs and we personalize each email with the person's name in the email. In order to prevent our admin from making a fool of himself, I'm adding in the template checkers. From our company perspective, we have concerns maybe our admin's finger slips and he types an extra closing bracket. So something like this:

Dear {{person_name}}}, Like to invite you to our event.

Then he accidentally sent a bad email to a ton of CEOs and it's embarrassing. So that's why I suggest maybe having a mode implemented for that, which I'd be happy to add as an optional feature, since I wrote the code for it on our product. The explicit error message is so when the admin tries to send a incorrectly templated email, a error pops up that is something like "Syntax Problem in your Email! undefined variable " rather than "Liquid error: undefined variable ." Just suggestions and like I said I'd be happy to implement the new setting if you guys feel it potentially adds value. If you want we can speak in a less public outlet, where I'd feel safer elaborating on the uses.

[dylanahsmith]

Liquid::Error#to_s has a with_prefix parameter that defaults to true. It is kind of weird to use, since calling error.to_s(false) isn't very clear at the call site. I think it would be more readable if we made it a keyword parameter.

[DexiongYung]

yea I'll try that. Let me know if you guys reconsider having a mode that does not consider extra closing brackets correct syntax, I have a bunch of regex written on our product that I'd be happy to give, cause that way it'll be handled by Liquid rather than me having a ton of code to handle it.