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

Issue with PluralLocalizationFormatter.TryEvaluateFormat in v3.2.0 #329

Closed
BtbN opened this issue Feb 18, 2023 · 7 comments · Fixed by #330
Closed

Issue with PluralLocalizationFormatter.TryEvaluateFormat in v3.2.0 #329

BtbN opened this issue Feb 18, 2023 · 7 comments · Fixed by #330
Assignees
Labels

Comments

@BtbN
Copy link

BtbN commented Feb 18, 2023

Given the following format string: {Game}{Category: [{}]|} by {Players} - {Hashtag}

Used like this:

var titleData = new
{
    options.Event.Hashtag,
    EventName = options.Event.Name,
    Game = relE.GameName,
    Category = relE.CategoryName,
    Players = lexPlayers,
    PlayerCount = relE.PlayerNamesTwitch.Count
};

video.Title = Smart.Format(options.Event.TitleTemplate, titleData);

This works perfectly fine with SmartFormat 3.1.0, but after upgrading to 3.2.0, it raises the following exception:

fail: VodManager.Services.VideoCombineService[0]
      Error while processing Run 1.
      SmartFormat.Core.Formatting.FormattingException: Error parsing format string: The input string 'Any% (Unrestricted)' was not in a correct format. at 16
      {Game}{Category: [{}]|} by {Players} - {Hashtag}
      ----------------^
       ---> System.FormatException: The input string 'Any% (Unrestricted)' was not in a correct format.
         at System.Number.ThrowOverflowOrFormatException(ParsingStatus status, ReadOnlySpan`1 value, TypeCode type)
         at System.Convert.ToDecimal(String value, IFormatProvider provider)
         at SmartFormat.Extensions.PluralLocalizationFormatter.TryEvaluateFormat(IFormattingInfo formattingInfo)
         at SmartFormat.SmartFormatter.InvokeFormatterExtensions(FormattingInfo formattingInfo)
         at SmartFormat.SmartFormatter.EvaluateFormatters(FormattingInfo formattingInfo)
         at SmartFormat.SmartFormatter.Format(FormattingInfo formattingInfo)
         --- End of inner exception stack trace ---
         at SmartFormat.SmartFormatter.FormatError(FormatItem errorItem, Exception innerException, Int32 startIndex, IFormattingInfo formattingInfo)
         at SmartFormat.SmartFormatter.Format(FormattingInfo formattingInfo)
         at SmartFormat.SmartFormatter.Format(FormatDetails formatDetails, Format format, Object current)
         at SmartFormat.SmartFormatter.Format(IFormatProvider provider, String format, IList`1 args)
         at SmartFormat.SmartFormatter.Format(String format, Object[] args)
         at SmartFormat.Smart.Format(String format, Object arg0)
         [...]

I'm not seeing anything in the 3.2.0 changelog that'd indicate a change like this, so this seems like a regression to me.
(Or the format string is wrong? It looks correct to me though, and worked fine for a really long time now.)

@axunonb
Copy link
Member

axunonb commented Feb 19, 2023

Hi, could you provide sample data that allows for figuring out the types that are included in the anonymous type for titleData?
And you're referencing the SmartFormat.NET package with PluralLocalizationFormatter included as a formatter extension?

@BtbN
Copy link
Author

BtbN commented Feb 19, 2023

It's all Strings, except for the Count, which is an int.
The Category can potentially be null, which is the whole point of its complex setup in the format string.

But it's really just the category part that causes troubles, so this is all it takes to trigger the exception:

Smart.Format("{Category: [{}]|}", new { Category = "test" });

@axunonb
Copy link
Member

axunonb commented Feb 19, 2023

I see, that's fine.

The format string is not wrong, but it (unintentionally) turned to be ambiguous with a fix to PluralLocalizationFormatter introduced in v3.2.0. Before {Category: [{}]|} was processed by ConditionalFormatter, while now PluralLocalizationFormatter kicks in.

The automatic formatter discovery is really nice and convenient, but there are edge cases where another formatter is selected than expected. We have 2 recommendations:

  • When using the SmartFormat.NET package, which includes almost the whole set of extensions, work with named formatters.
  • Try to enable only these extensions, which are actually required: Less potential ambiguity, higher performance.

3 simple options to resolve your issue:

  • Smart.Default.RemoveFormatterExtension<PluralLocalizationFormatter>(); - you're not using it anyway
  • Use a modified format string: {Game}{Category:cond: [{}]|} by {Players} - {Hashtag} select theConditionalFormatter at it was before
  • Use a modified format string: {Game}{Category:isnull: | [{}]} by {Players} - {Hashtag} select the NullFormatter. I would prefer this one, because the formatter name clearly says that it's about handling null.

@axunonb
Copy link
Member

axunonb commented Feb 19, 2023

Also, we'll improve auto-detection in PluralLocalizationFormatter.TryEvaluateFormat() of course.

axunonb added a commit to axunonb/SmartFormat that referenced this issue Feb 19, 2023
Current behavior, introduced in v3.2.0:
When PluralLocalizationFormatter.CanAutoDetect == true, values not convertible to decimal will throw then trying to IConvertible.ToDecimal(...)

New behavior:
When PluralLocalizationFormatter.CanAutoDetect == true, for values not convertible to decimal, IFormatter.TryEvaluateFormat(...) will return false
@axunonb axunonb added Bug and removed Investigate labels Feb 19, 2023
@axunonb axunonb self-assigned this Feb 19, 2023
@axunonb axunonb changed the title Format parsing regression in 3.2.0 Issue with PluralLocalizationFormatter.TryEvaluateFormat in v3.2.0 Feb 19, 2023
@axunonb
Copy link
Member

axunonb commented Feb 19, 2023

Maybe you'd like to evaluate the build of the latest PR

axunonb added a commit that referenced this issue Feb 19, 2023
… values not convertible to decimal (#330)

* Resolves #329

Current behavior, introduced in v3.2.0:
When PluralLocalizationFormatter.CanAutoDetect == true, values not convertible to decimal will throw then trying to IConvertible.ToDecimal(...)

New behavior:
When PluralLocalizationFormatter.CanAutoDetect == true, for values not convertible to decimal, IFormatter.TryEvaluateFormat(...) will return false

* Make PluralLocalizationFormatter.TryGetDecimalValue static
@BtbN
Copy link
Author

BtbN commented Feb 19, 2023

Seems like I was a bit late.
Just tested it, and the issue is indeed gone!
Thank you for the quick fix :)

@axunonb
Copy link
Member

axunonb commented Feb 19, 2023

Not at all, thanks a lot for your hint 👍
Still, our recommendation with named formatters is still valid 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants