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

PluralLocalizationFormatter should not try to format pure numeric strings #345

Closed
axunonb opened this issue Aug 1, 2023 · 3 comments
Closed
Assignees

Comments

@axunonb
Copy link
Member

axunonb commented Aug 1, 2023

In version v3.1.0 and before, the PluralLocalizationFormatter rejected to format bool or string values.

When implicit formatter detection is used - i.e. not named formatters - the following format string was processed by ConditionalFormatter. This is considered as the desired and expected behavior:

Smart.Format("Arg value: {0:{} |is null or empty}", default(object?));
// returns "Arg value: is null or empty"
Smart.Format("Arg value: {0:{} |is null or empty}", "1234");
// returns "Arg value: 1234 "

This behavior changed in v3.2.0.

Smart.Format("Arg value: {0:{} |is null or empty}", "1234");
// returns unexpected "Arg value: is null or empty"

// Using the named formatter:
 Smart.Format("Arg value: {0:cond:{} |is null or empty}", "1234")
// returns "Arg value: 1234 "

Reason for the changed result: PluralLocalizationFormatter kicked in as the first formatter that can handle the format string and arg value.

A workaround for this specific format string was:

Smart.Default.GetFormatterExtension<PluralLocalizationFormatter>().CanAutoDetect = false;

Conclusion:
The behavior of PluralLocalizationFormatter should be reverted to the behavior of version v3.1.0 and before.
Additional unit tests for this use case should be added.

Note:
Implicit formatter detection is comfortable, but is also at risk not detecting the expected formatter.
Using namend formatters is always more safe.

See also this discussion: #344
@sygmond

@axunonb axunonb self-assigned this Aug 1, 2023
@sygmond
Copy link

sygmond commented Aug 1, 2023

Thank you for nicely layout examples and explanation. I understand it now better.

@axunonb
Copy link
Member Author

axunonb commented Aug 1, 2023

@karljj1 The issue is because a string is also IConvertible, but shouldn't be processed by PluralLocalizationFormatter. Agree?

@axunonb axunonb changed the title PluralLocalizationFormatter should not try to format boolean or string values PluralLocalizationFormatter should not try to format pure numeric strings Aug 1, 2023
@karljj1
Copy link
Collaborator

karljj1 commented Aug 2, 2023

@karljj1 The issue is because a string is also IConvertible, but shouldn't be processed by PluralLocalizationFormatter. Agree?

Yes that does make sense. I recall adding an exception for bools for a similar reason, I guess we also need one for strings.

axunonb added a commit to axunonb/SmartFormat that referenced this issue Aug 2, 2023
* PluralLocalizationFormatter does treat numeric string as valid argument
* Restore behavior of v3.1.0 and before
* Bump version to v3.2.2
* Update appveyor and github workflow CI
@axunonb axunonb closed this as completed in e98917a Aug 3, 2023
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

No branches or pull requests

3 participants