search

axuno / SmartFormat

A lightweight text templating library written in C# which can be a drop-in replacement for string.Format
Other
1.1k stars 105 forks source link

FormatErrorAction ignored in 2.5.0 #125

Closed j055 closed 4 years ago

j055 commented 4 years ago

Error handling does not work as expected in 2.5.0.

Smart.Default.Settings.FormatErrorAction = ErrorAction.Ignore;
var source = "{";
string html = SmartFormat.Smart.Format(source, new object());

Throws exception:

SmartFormat.Core.Parsing.ParsingErrors: The format string has 1 issue:
Format string is missing a closing brace
In: "{"
At:  -^ 
   at SmartFormat.Core.Parsing.Parser.ParseFormat(String format, String[] formatterExtensionNames) in D:\Documents\Visual Studio 2019\Projects\SmartFormat\src\SmartFormat\Core\Parsing\Parser.cs:line 449
   at SmartFormat.SmartFormatter.Format(IFormatProvider provider, String format, Object[] args) in D:\Documents\Visual Studio 2019\Projects\SmartFormat\src\SmartFormat\SmartFormatter.cs:line 149
   at SmartFormat.SmartFormatter.Format(String format, Object[] args) in D:\Documents\Visual Studio 2019\Projects\SmartFormat\src\SmartFormat\SmartFormatter.cs:line 135
   at SmartFormat.Smart.Format(String format, Object arg0) in D:\Documents\Visual Studio 2019\Projects\SmartFormat\src\SmartFormat\Smart.cs:line 41
   at UserQuery.Main()

The behaviour works as expected in 2.4.2

j055 commented 4 years ago

I have no problem with Default ErrorAction is now ThrowError

In my example I am setting:

Smart.Default.Settings.FormatErrorAction = ErrorAction.Ignore;

Actually in our production code we normally use ErrorAction.MaintainTokens

It looks like setting FormatErrorAction to any value has no effect and always throws an error.

We still need the option.

From: Norbert Bietsch notifications@github.com Sent: 16 January 2020 13:56 To: axuno/SmartFormat SmartFormat@noreply.github.com Cc: Andrew Jocelyn andrew.jocelyn@empetus.co.uk; Author author@noreply.github.com Subject: Re: [axuno/SmartFormat] FormatErrorAction ignored in 2.5.0 (#125)

Yes, you are right. As pointed out in the release notes

Breaking Change: Default ErrorAction is now ThrowError for parser and formatter, instead of Ignore

This means exacly what you describe: The source string contains an error, a missing closing brace, namely. There were many issues reported because of ErrorAction.Ignore. Actually we think it is rarely a good idea just to ignore errors, also because of unpredictable outcome. Would you agree? Which disadvantages do you experience?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/axuno/SmartFormat/issues/125?email_source=notifications&email_token=ABN3UJHIIWNNEPBUYECMMFTQ6BROPA5CNFSM4KHRSIH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJEENJI#issuecomment-575162021, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABN3UJGRU3IY7I6BD4DJ5L3Q6BROPANCNFSM4KHRSIHQ.

axunonb commented 4 years ago

Ah, I see. Will investigate. Wondering why unit tests didn't fail here. Will come back shortly.

j055 commented 4 years ago

My bad, I didn't set

Smart.Default.Settings.ParseErrorAction, only Smart.Default.Settings.FormatErrorAction

I didn't know there was a ParseErrorAction setting until I looked at the docs. I guess this further endorses the decision to make ThrowError the default. I probably would have paid attention to these settings when I started using SmartFormat all those years ago if it had been the default then.

Thank you

axunonb commented 4 years ago

Very good to see that the docs could resolve your issue ;-) Thanks for the clarification.

neclamis1285 commented 4 years ago

Am I missing something? I tried both settings being on and just parseError on and still I am getting an error.

// Smart.Default.Settings.FormatErrorAction = ErrorAction.Ignore; Smart.Default.Settings.ParseErrorAction = ErrorAction.Ignore; var emailTemplate = Smart.Format(File.ReadAllText(templatePath), values);

axunonb commented 4 years ago

Could you please post a snippet where I could reproduce your issue? Which error do you get?

neclamis1285 commented 4 years ago

Sure, Here is the code sample and the error

var cssString = "{padding: 10px; margin: 1rem} {Website}"; Smart.Default.Settings.FormatErrorAction = ErrorAction.MaintainTokens; Smart.Default.Settings.ParseErrorAction = ErrorAction.MaintainTokens; var emailTemplate = Smart.Format(cssString, values);

The error I received is

formattingInfo.FormattingException("Could not evaluate the selector \"" + selector.RawText + "\"", (FormatItem) selector, -1);

axunonb commented 4 years ago

This code does not throw an exception and the "values" variable is not defined. But still:

var cssString = "{padding: 10px; margin: 1rem} {Website}"; 
Smart.Default.Settings.FormatErrorAction = ErrorAction.MaintainTokens; 
Smart.Default.Settings.ParseErrorAction = ErrorAction.MaintainTokens;
var emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});
// result as expected: "{padding: 10px; margin: 1rem} axuno.net"
Smart.Default.Settings.FormatErrorAction = ErrorAction.Ignore; 
Smart.Default.Settings.ParseErrorAction = ErrorAction.Ignore;
emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});
// result as expected: " axuno.net"
Smart.Default.Settings.FormatErrorAction = ErrorAction.ThrowError; 
Smart.Default.Settings.ParseErrorAction = ErrorAction.ThrowError;
emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});
// result as expected: SmartFormat.Core.Formatting.FormattingException: 'Error parsing format string: Could not evaluate the selector "padding" at 1 {padding: 10px; margin: 1rem} {Website} -^'

Your issue is caused by not escaping the curly braces of the CSS part. This is the solution:

cssString = @"\{padding: 10px; margin: 1rem\} {Website}"; 
emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});
// result: "{padding: 10px; margin: 1rem} axuno.net"

Correct?

neclamis1285 commented 4 years ago

Sorry, I did not include the complete code where values was defined. None the less, I thought that by setting the two settings:

Smart.Default.Settings.FormatErrorAction = ErrorAction.MaintainTokens; Smart.Default.Settings.ParseErrorAction = ErrorAction.MaintainTokens;

That I was not supposed to get an error regardless of whether or not I did not escape the CSS part. I use MJML to create email templates and they inline the CSS which can get to be a lot so I just wanted to ignore variables that could not be found. Is that not the reason for the error settings? Am I misunderstanding something?

Also I am not sure why you did not get an error because I have tried it several times and it always throws an error.

On Sun, May 17, 2020 at 3:19 PM Norbert Bietsch notifications@github.com wrote:

This code does not throw an exception and the "values" variable is not defined. But still:

var cssString = "{padding: 10px; margin: 1rem} {Website}"; Smart.Default.Settings.FormatErrorAction = ErrorAction.MaintainTokens; Smart.Default.Settings.ParseErrorAction = ErrorAction.MaintainTokens;var emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});// result as expected: "{padding: 10px; margin: 1rem} axuno.net"Smart.Default.Settings.FormatErrorAction = ErrorAction.Ignore; Smart.Default.Settings.ParseErrorAction = ErrorAction.Ignore;emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});// result as expected: " axuno.net"Smart.Default.Settings.FormatErrorAction = ErrorAction.ThrowError; Smart.Default.Settings.ParseErrorAction = ErrorAction.ThrowError;emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});// result as expected: SmartFormat.Core.Formatting.FormattingException: 'Error parsing format string: Could not evaluate the selector "padding" at 1 {padding: 10px; margin: 1rem} {Website} -^'

Your issue is caused by not escaping the curly braces of the CSS part. This is the solution:

cssString = @"{padding: 10px; margin: 1rem} {Website}"; emailTemplate = Smart.Format(cssString, new {Website = "axuno.net"});// result: "{padding: 10px; margin: 1rem} axuno.net"

Correct?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/axuno/SmartFormat/issues/125#issuecomment-629869384, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7TPIU3YNCEUAY2KXGDGPTRSBPGBANCNFSM4KHRSIHQ .

-- Sincerely, Bernard Chung (623) 252-6476

axunonb commented 4 years ago

Bernard

Is that not the reason for the error settings?

Although it looks like the reason, actually we did not think about keeping CSS with this setting. In fact, you could achieve what you intend to.

Also I am not sure why you did not get an error

As you see in our sample code, we do get errors with ErrorAction.ThrowError set. Other settings will not throw by design. The SmartFormat unit tests have sample code to understand the concept.

When using MJML we see 2 things to take care of: Integrated style tags with CSS, and integrated <script> tags. We recommend not to parse such tag content with SmartFormat. These tags can be identified easily with an HTML parser like AngleSharp.