dotliquid / dotliquid

.NET Port of Tobias Lütke's Liquid template language.
http://dotliquidmarkup.org
Other
1.04k stars 299 forks source link

Fix Param Whitespace Throws Exception #526

Closed microalps closed 1 year ago

microalps commented 1 year ago

Resolves #496

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (b2fe268) 88.34% compared to head (37500e9) 88.34%.

:exclamation: Current head 37500e9 differs from pull request most recent head 8c472ce. Consider uploading reports for the commit 8c472ce to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #526 +/- ## ======================================= Coverage 88.34% 88.34% ======================================= Files 69 69 Lines 2694 2694 Branches 631 631 ======================================= Hits 2380 2380 Misses 210 210 Partials 104 104 ``` | [Impacted Files](https://codecov.io/gh/dotliquid/dotliquid/pull/526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotliquid) | Coverage Δ | | |---|---|---| | [src/DotLiquid/Tags/Param.cs](https://codecov.io/gh/dotliquid/dotliquid/pull/526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotliquid#diff-c3JjL0RvdExpcXVpZC9UYWdzL1BhcmFtLmNz) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotliquid). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotliquid)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

microalps commented 1 year ago

The solution is a bit hackish because the following filter is silently ignored: {% param Syntax = 'DotLiquid21' | replace: '21', '10' %} I don't think we want to support filters here. Simple trim will pass entire string with filters to context, which will obviously fail but without any error handling or messaging that filters are not supported. Suggestions?

daviburg commented 1 year ago

The solution is a bit hackish because the following filter is silently ignored: {% param Syntax = 'DotLiquid21' | replace: '21', '10' %} I don't think we want to support filters here. Simple trim will pass entire string with filters to context, which will obviously fail but without any error handling or messaging that filters are not supported. Suggestions?

With simple string trim {% param Syntax = 'DotLiquid21' | replace: '21', '10' %} will turn into {% param Syntax = 'DotLiquid21' | replace: '21', '10'%}, yes? What error does user get with this unsupported filter?

Are there scenarios where string trim doesn't fix a whitespace issue but the ().Name hack does fix?

microalps commented 1 year ago

@daviburg

What error does user get with this unsupported filter?

Liquid syntax error: Invalid syntax option: 'DotLiquid21' | r...

Are there scenarios where string trim doesn't fix a whitespace issue but the ().Name hack does fix?

Not that I know of. Reran the tests, they all passed.

daviburg commented 1 year ago

@daviburg

What error does user get with this unsupported filter?

Liquid syntax error: Invalid syntax option: 'DotLiquid21' | r...

Are there scenarios where string trim doesn't fix a whitespace issue but the ().Name hack does fix?

Not that I know of. Reran the tests, they all passed.

Then string trim seems a pretty clean option.