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

Added StringComparer property to TemplateContext #681

Merged
merged 21 commits into from
Sep 30, 2024

Conversation

gumbarros
Copy link
Contributor

@gumbarros gumbarros commented Jul 12, 2024

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

Fluid.Tests/TemplateContextTests.cs Show resolved Hide resolved
Fluid/Scope.cs Outdated Show resolved Hide resolved
Fluid/Scope.cs Outdated Show resolved Hide resolved
Fluid/Scope.cs Outdated Show resolved Hide resolved
gumbarros and others added 5 commits July 12, 2024 21:03
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
Co-authored-by: Hisham Bin Ateya <hishamco_2007@yahoo.com>
Fluid/TemplateContext.cs Outdated Show resolved Hide resolved
@gumbarros gumbarros changed the title Added StringComparer property to TemplateContext and TemplateOptions Added StringComparer property to TemplateOptions Jul 13, 2024
Fluid/TemplateContext.cs Outdated Show resolved Hide resolved
Fluid/TemplateOptions.cs Outdated Show resolved Hide resolved
@gumbarros gumbarros changed the title Added StringComparer property to TemplateOptions Added StringComparer property to TemplateContext Jul 13, 2024
@gumbarros gumbarros requested a review from hishamco July 13, 2024 12:37
@hishamco
Copy link
Collaborator

@gumbarros check my last commit

@gumbarros
Copy link
Contributor Author

@gumbarros check my last commit

LGTM

Fluid/Scope.cs Outdated Show resolved Hide resolved
@hishamco
Copy link
Collaborator

@sebastienros anything else to add to this?

@hishamco
Copy link
Collaborator

Thanks for a quick fix :)

@gumbarros
Copy link
Contributor Author

Any plans to merge this 👀?

@gumbarros
Copy link
Contributor Author

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
Copy link
Collaborator

hishamco commented Aug 2, 2024

@sebastienros any feedback on this or shall I merge?

@sebastienros
Copy link
Owner

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
Copy link
Owner

@gumbarros

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

@hishamco
Copy link
Collaborator

hishamco commented Aug 2, 2024

To be honest it was as it's seen in b99e0f5

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

@sebastienros
Copy link
Owner

To be honest it was as it's seen in b99e0f5

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

@hishamco
Copy link
Collaborator

hishamco commented Aug 3, 2024

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
Copy link
Owner

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
Copy link
Collaborator

hishamco commented Aug 3, 2024

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
Copy link
Contributor Author

Hi guys, any news on merging this 👀?

@sebastienros
Copy link
Owner

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)

@sebastienros sebastienros merged commit f5af4c8 into sebastienros:main Sep 30, 2024
1 check passed
@nguyenngocanhpro
Copy link

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
Copy link
Owner

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

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.

4 participants