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

Fix parsing issues #149

Merged
merged 5 commits into from
Apr 9, 2021
Merged

Fix parsing issues #149

merged 5 commits into from
Apr 9, 2021

Conversation

axunonb
Copy link
Member

@axunonb axunonb commented Apr 2, 2021

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:

  1. Parser.ErrorAction.Ignore: Only Placeholders with ParsingErrors become string.Empty, others are formatted.
  2. 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.

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

@codecov
Copy link

codecov bot commented Apr 2, 2021

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
Impacted file tree graph

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

andersjonsson commented Apr 9, 2021

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

Your work on this issue is much appreciated! 👍

@axunonb
Copy link
Member Author

axunonb commented Apr 9, 2021

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

axunonb commented Apr 9, 2021

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

andersjonsson commented Apr 9, 2021

// 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
Copy link
Member Author

axunonb commented Apr 9, 2021

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.

@axunonb axunonb marked this pull request as ready for review April 9, 2021 21:26
@axunonb axunonb merged commit d383f63 into main Apr 9, 2021
@axunonb axunonb deleted the Fix_parsing_issues branch April 9, 2021 21:29
axunonb added a commit that referenced this pull request 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants