Fix parsing issues #149

axunonb · 2021-04-02T18:50:00Z

This PR addresses issues referenced in #148, #147, #143.

Also fixed:
If the last item of a format string was an uncompleted Placeholder (e.g. ending "like {uncomplete"), the parser now takes it as an erroneous Placeholder, not as a TextLiteral (as it was the case until v2.6.2).

@andersjonsson @nikita-starostin @hugoqribeiro: Your feedback on this PR would be appreciated.

Here is the draft package of SmartFormat.NET.2.7.0-preview-1 (not published yet, more to come):
SmartFormat.NET.2.7.0-preview-1.zip

@andersjonsson @nikita-starostin Since v2.6.2 the following issue is breaking backward-compatibility:

Parser.ErrorAction.MaintainTokens: If one of serveral Placeholders has ParsingErrors, all Placeholders are just displayed with their tokens. Expected: Only Placeholders with ParsingErrors should show up with their tokens, others should be formatted.

This has been fixed,

Other issues with the Parser have been fixed, too:

Parser.ErrorAction.Ignore: Only Placeholders with ParsingErrors become string.Empty, others are formatted.
Since v1.6.1 there is an undiscovered issue: If Parser found a ParsingError.TooManyClosingBraces, this closing brace was simply "swallowed-up". Expected: The redundant closing brace should be treated as a literal. Otherwise the result with Parser.ErrorAction.MaintainTokens has differences to the original format string.

@hugoqribeiro @nikita-starostin: You are using SmartFormat.NET for processing HTML strings with JavaScript or CSS.

With the fixes described above, chances for SmartFormat.NET to succeed, will increase.
The following tests will make clear, why this is not reliable, and this unfortunately cannot be improved further:

SmartFormat/src/SmartFormat.Tests/Core/ParserTests.cs

Line 172 in ced05bb

public void Parse_JavaScript_May_Succeed_Or_Fail(string var0, string var1, bool shouldSucceed)
SmartFormat/src/SmartFormat.Tests/Core/ParserTests.cs

Line 260 in ced05bb

public void Parse_Css_May_Succeed_Or_Fail(string var0, string var1, bool shouldSucceed)

…rmat into Fix_parsing_issues

codecov · 2021-04-02T18:50:55Z

Codecov Report

Merging #149 (7a6e8ce) into main (2ae8c7f) will decrease coverage by 0.01%.
The diff coverage is 86.95%.

❗ Current head 7a6e8ce differs from pull request most recent head 2c542c9. Consider uploading reports for the commit 2c542c9 to get more accurate results

@@            Coverage Diff             @@
##             main     #149      +/-   ##
==========================================
- Coverage   94.41%   94.39%   -0.02%     
==========================================
  Files          42       42              
  Lines        1755     1749       -6     
==========================================
- Hits         1657     1651       -6     
  Misses         98       98

Impacted Files	Coverage Δ
src/SmartFormat/Core/Parsing/Parser.cs	`98.61% <84.21%> (+0.01%)`	⬆️
src/SmartFormat/Core/Parsing/LiteralText.cs	`100.00% <100.00%> (ø)`
src/SmartFormat/Core/Parsing/ParsingErrors.cs	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae8c7f...2c542c9. Read the comment docs.

andersjonsson · 2021-04-09T11:40:19Z

Great job!
Sorry about the delay, checking it out now.

I'm seeing some weirdness:

var data = new { Name = "Anders Jonsson", City = "Stockholm" };

Smart.Default.Settings.ParseErrorAction = SmartFormat.Core.Settings.ErrorAction.MaintainTokens;
Smart.Format("Hello {Name from {City}", data).Dump();
//Expected: "Hello {Name from Stockholm"
//Result: "Hello {Name from {City}"


Smart.Format("Hello {City}, I'm {Name", data).Dump();
//Expected: "Hello Stockholm, I'm {Name"
//Result: "Hello Stockholm, I'm {NameName"

Are my expectations in line with what you are aiming for? Regardless, the last one seems unexpected 😉

andersjonsson · 2021-04-09T11:40:47Z

Your work on this issue is much appreciated! 👍

axunonb · 2021-04-09T18:15:59Z

Regardless, the last one seems unexpected

Indeed, thank you. This case was not explicitly covered in a unit tests (added now)

Actual behavior:
If the last item of a format string is an uncompleted Placeholder, it is parsed as LiteralText

Expected behavior:
If the last item of a format string is an uncompleted Placeholder, it should be still be parsed as Placeholder

This should be fixed now:
SmartFormat.NET.2.7.0-preview-2.zip

axunonb · 2021-04-09T18:33:12Z

// Expected: "Hello {Name from Stockholm"

Here, the parser takes all between first opening and closing brace as Placeholder. Starting to "guess" would become really complex if we're considering more challenging cases than this simple one. {Name from {{City} { and {City}} 😈

andersjonsson · 2021-04-09T19:02:34Z

// Expected: "Hello {Name from Stockholm"

Here, the parser takes all between first opening and closing brace as Placeholder. Starting to "guess" would become really complex if we're considering more challenging cases than this simple one. {Name from {{City} { and {City}} 😈

Agreed. Yeah, i’ve written some parsers and it can get really messy.
That bit can always be revisited later on 😉

Your PR sure adds a lot of value as-is. Great job!

axunonb · 2021-04-09T19:15:53Z

i’ve written some parsers

Ooohh, how wonderful! Stay tuned 😁

In the end, the current parser must be re-written from scratch, as it's essentially unchanged since v1.6 or longer:
Should be less complex, less limitations also in terms of extensions, less GC when creating the strings...
Heading for v3.

* Reference to issues #148, #147, #143 * Updated README.md * Fix for #149 (comment) * Update CHANGES.md

axunonb added 4 commits April 2, 2021 19:22

Reference to issues #148, #147, #143

ced05bb

Reference to issues #148, #147, #143

3d80b53

Merge branch 'Fix_parsing_issues' of https://github.com/axuno/SmartFo…

c33c94a

…rmat into Fix_parsing_issues

Updated README.md

1ed6107

axunonb mentioned this pull request Apr 2, 2021

Different behavior in 2.6.2 vs 2.5.0 for ErrorAction.MaintainTokens #148

Closed

Fix for #149 (comment)

2c542c9

axunonb marked this pull request as ready for review April 9, 2021 21:26

axunonb merged commit d383f63 into main Apr 9, 2021

axunonb deleted the Fix_parsing_issues branch April 9, 2021 21:29

axunonb added a commit that referenced this pull request Apr 10, 2021

Fix parsing issues (#152)

cbf3d3b

* Reference to issues #148, #147, #143 * Updated README.md * Fix for #149 (comment) * Update CHANGES.md

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing issues #149

Fix parsing issues #149

axunonb commented Apr 2, 2021 •

edited

Loading

codecov bot commented Apr 2, 2021 •

edited

Loading

andersjonsson commented Apr 9, 2021 •

edited

Loading

andersjonsson commented Apr 9, 2021

axunonb commented Apr 9, 2021

axunonb commented Apr 9, 2021 •

edited

Loading

andersjonsson commented Apr 9, 2021 •

edited

Loading

axunonb commented Apr 9, 2021

Fix parsing issues #149

Fix parsing issues #149

Conversation

axunonb commented Apr 2, 2021 • edited Loading

codecov bot commented Apr 2, 2021 • edited Loading

Codecov Report

andersjonsson commented Apr 9, 2021 • edited Loading

andersjonsson commented Apr 9, 2021

axunonb commented Apr 9, 2021

axunonb commented Apr 9, 2021 • edited Loading

andersjonsson commented Apr 9, 2021 • edited Loading

axunonb commented Apr 9, 2021

axunonb commented Apr 2, 2021 •

edited

Loading

codecov bot commented Apr 2, 2021 •

edited

Loading

andersjonsson commented Apr 9, 2021 •

edited

Loading

axunonb commented Apr 9, 2021 •

edited

Loading

andersjonsson commented Apr 9, 2021 •

edited

Loading