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

Entire data object is included in output on ParseError, when ParseErrorAction is set to anything other than ThrowError #143

Closed
andersjonsson opened this issue Feb 15, 2021 · 10 comments · Fixed by #145
Labels

Comments

@andersjonsson
Copy link
Contributor

andersjonsson commented Feb 15, 2021

Template: "Hello {Name"

That template is clearly missing an end curly brace

Smart.Format("Hello {Name", new { Name = "Anders Jonsson", City = "Stockholm"})
//Result: Exception

So far so good.

But after setting Smart.Default.Settings.ParseErrorAction I get an unexpected output.

Smart.Default.Settings.ParseErrorAction = SmartFormat.Core.Settings.ErrorAction.Ignore;
Smart.Format("Hello {Name", new { Name = "Anders Jonsson", City = "Stockholm"})
//Result: "Hello { Name = Anders Jonsson, City = Stockholm }Name"

Smart.Default.Settings.ParseErrorAction = SmartFormat.Core.Settings.ErrorAction.MaintainTokens;
Smart.Format("Hello {Name", new { Name = "Anders Jonsson", City = "Stockholm"})
//Result: "Hello { Name = Anders Jonsson, City = Stockholm }Name"

Smart.Default.Settings.ParseErrorAction = SmartFormat.Core.Settings.ErrorAction.OutputErrorInResult;
Smart.Format("Hello {Name", new { Name = "Anders Jonsson", City = "Stockholm"})
//Result: "Hello { Name = Anders Jonsson, City = Stockholm }Name"

That doesn't seem like the intended behavior. Am I missing something?

@andersjonsson andersjonsson changed the title Entire data object is included in output on ParseError, when ParseErrorAction is set to Ignore or MaintainTokens Entire data object is included in output on ParseError, when ParseErrorAction is set to anything other than ThrowError Feb 15, 2021
@axunonb axunonb added the Bug label Feb 15, 2021
@axunonb
Copy link
Member

axunonb commented Feb 16, 2021

Hi,

Looks like for ParseErrorAction only ErrorAction.ThrowError was implemented.
With other settings, the formatter is still called, leading to unexpected results like in your sample.

So, yes, the other error actions could be implemented. I'm a bit in doubt, whether anything else than ThrowError makes sense for parsing errors, though.

When implemented, in your example, the results would look like this:

MaintainTokens: Hello {Name

Ignore: (an empty string)

OutputErrorInResult: The format string has 1 issue:
Format string is missing a closing brace
In: "Hello {Name"
At: -----------^

ThrowException:
Well, throws...

What do you think?

@andersjonsson
Copy link
Contributor Author

andersjonsson commented Feb 16, 2021

I think your proposed result for MaintainTokens makes sense.
I personally want a behavior where SmartFormat does what it can with the input and leaves the rest the way that it was.

Regarding Ignore I would expect the result to be Hello , but I see your point. A case could even be made for Hello Name, but I think that's a stretch.

@axunonb
Copy link
Member

axunonb commented Feb 16, 2021

Regarding Ignore I would

This kind of Ignore would also be close to MaintainTokens, and goes away from "don't show the problem to possible end users".
What about proceeding the way mentioned above? I think I could finish the implementation tomorrow. Would you be able to provide the required unit tests on top? Then I would open a new branch for that.

@andersjonsson
Copy link
Contributor Author

What about proceeding the way mentioned above?

I see you point. Sounds good!

Would you be able to provide the required unit tests on top?

Sure thing

@axunonb
Copy link
Member

axunonb commented Feb 16, 2021

Sure thing

Great! Should go into ParserTests.cs

@andersjonsson
Copy link
Contributor Author

Implementing the tests now.

When I use the template Hello, I'm {Name from {City}
and the input new { Name = "John", City = "Oklahoma" }

MaintainTokens give me Hello, I'm {Name from {City}. I was expecting Hello, I'm {Name from Oklahoma

And the parser error says "SmartFormat.Core.Parsing.ParsingErrors : The format string has 3 issues:
Invalid character in the selector, Invalid character in the selector, Invalid character in the selector"

With Hello, I'm {Name it says "SmartFormat.Core.Parsing.ParsingErrors : The format string has 1 issue:
Format string is missing a closing brace". A more correct description of the problem IMHO.

Is it possible to make the parser leave the current expression on the first error, so that it can successfully parse correct expressions later in the string?

@axunonb
Copy link
Member

axunonb commented Feb 17, 2021 via email

@axunonb
Copy link
Member

axunonb commented Feb 17, 2021

Is it possible to make the parser leave the current expression on the first error, so that it can successfully parse correct expressions later in the string?

Short answer: no
Long answer: would require a major redesign, and it also would be difficult to evaluate the rest of the string if there is an error in it before

@andersjonsson
Copy link
Contributor Author

Short answer: no
Long answer: would require a major redesign, and it also would be difficult to evaluate the rest of the string if there is an error in it before

Oh, that's too bad. Now I see why you didn't think anything else than ThrowError made sense. I guess I should have had a better example in my initial description.

I'll send a PR with the tests shortly

@axunonb
Copy link
Member

axunonb commented Feb 17, 2021

Thanks a lot. Will be able to have a deeper look tonight (CET).

axunonb added a commit that referenced this issue Apr 2, 2021
axunonb added a commit that referenced this issue Apr 2, 2021
axunonb added a commit that referenced this issue Apr 9, 2021
* Reference to issues #148, #147, #143

* Reference to issues #148, #147, #143

* Updated README.md

* Fix for #149 (comment)
axunonb added a commit that referenced this issue Apr 10, 2021
* Reference to issues #148, #147, #143
* Updated README.md
* Fix for #149 (comment)
* Update CHANGES.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants