Added `StringComparer` property to `TemplateContext`

[gumbarros]

This PR allows case insensitivity at templates, making more easy to non-programmers to use variables and functions.

[hishamco]

@gumbarros check my last commit

[gumbarros]

@gumbarros check my last commit

LGTM

[hishamco]

@sebastienros anything else to add to this?

[hishamco]

Thanks for a quick fix :)

[gumbarros]

Any plans to merge this 👀?

[gumbarros]

I'm going to be out for 20 days next week and won't be able to make any changes for this PR. I would also like to use this feature this week if possible.

[hishamco]

@sebastienros any feedback on this or shall I merge?

[sebastienros]

This is too intrusive (in the ctor). Can this StringComparer be yet another property of the options/context classes, be inherited like the other properties?

And the new property should be named ModelNamesComparer? The default value should be Ordinal obviously.

[sebastienros]

@gumbarros

if you can't do the changes @hishamco or myself should be able to do it.

[hishamco]

To be honest it was as it's seen in https://github.com/sebastienros/fluid/pull/681/commits/b99e0f54853fbebf42101758f24388917e2ac32f

If you like how it was I will revert the latest changes considering the name that you suggested

[sebastienros]

To be honest it was as it's seen in https://github.com/sebastienros/fluid/commit/b99e0f54853fbebf42101758f24388917e2ac32f

Did I actually ask the opposite, or was it done on your own thoughts?

[hishamco]

It added as property at the beginning but I saw the property appears twice in different places that's why I suggested to pass the values through constructor

[sebastienros]

In you case that's what you mean, it's pretty common to see the properties duplicated and cloned from TemplateOptions and TemplateContext. But this is done this way for anything that can alter the behavior or the rendering, to have defaults or options to share across template content instances without having to set it each time.

[hishamco]

I see if the linked commit is fine for you I will revert the latest changes then merge unless you need to add something else

[gumbarros]

Hi guys, any news on merging this 👀?

[sebastienros]

Updated it. Should be simpler, and it helped me realize that this property can't be changed dynamically as it is used by an inner collection.

In Fluid 3.0 we might want to use the same comparer for both internal properties and model properties. (note for myself)

[nguyenngocanhpro]

When will this PR be released? I've been waiting for so long. And I think the GetValueAsync function from FluidValue should be updated in the same way as this case

[sebastienros]

@nguyenngocanhpro do you want to submit a PR for GetValueAsync? I can ship it right after that.

[nairdo]

With this PR (tested using v2.24.0), should we still be able to use case-sensitive variables if the TemplateOptions ModelNamesComparer is set to StringComparer.Ordinal?

I was under the impression that Shopify Liquid still supports this behavior (example):

 {% assign case = "lower" %} 
 {% assign CASE = "upper" %} 
 case = {{ case }} <!-- expect lower -->
 CASE = {{ CASE }} <!-- expect upper -->

Here's a unit test:

[Fact]
public void ShouldUseTemplateOptionsStringComparerWithCaseSensitive()
{
   var options = new TemplateOptions { ModelNamesComparer = StringComparer.Ordinal };
   var context = new TemplateContext(options);
   context.SetValue("case", "lower");
   context.SetValue("CASE", "upper");
   context.SetValue("Case", "mixed");

   Assert.Equal("lowerupper", context.GetValue("case").ToStringValue() + context.GetValue("CASE").ToStringValue());
}

Results:

Actual: "mixedmixed"

[sebastienros]

The link you provided is from liquidjs, not shopify, here is the result from Shopify (https://shopify.dev/docs/api/liquid)

[nairdo]

@sebastienros Right, sorry about using liquidjs, I just mainly wanted to confirm that Fluid should still allow case sensitivity. I'll open this as a new issue.

[sebastienros]

I just wanted to be sure it was also the behavior in shopify. I also create issues on liquidjs because it doesn't follow the standard so I wanted to point it out.

[sebastienros]

Here is the PR fixing the bug: https://github.com/sebastienros/fluid/pull/802