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

Ignore bools in plurals formatter. #306

Merged
merged 1 commit into from
Aug 21, 2022

Conversation

karljj1
Copy link
Collaborator

@karljj1 karljj1 commented Aug 12, 2022

I introduced a small bug in my last PR. The plurals formatter will now also consider bools which changes the behaviors when it is using auto detect. This was causing some of our tests to fail because we had an Arabic language running at the time which produces different plurals.

@@ -113,7 +113,7 @@ public bool TryEvaluateFormat(IFormattingInfo formattingInfo)
// We can format numbers, and IEnumerables. For IEnumerables we look at the number of items
// in the collection: this means the user can e.g. use the same parameter for both plural and list, for example
// 'Smart.Format("The following {0:plural:person is|people are} impressed: {0:list:{}|, |, and}", new[] { "bob", "alice" });'
if (current is IConvertible convertible)
if (current is IConvertible convertible && current is not bool)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do still consider Enums which we did not previously although that seemed fine to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a unit test for that case 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess, I oversaw an my mobile, that the test is already there.
Thanks a lot for submitting the fix.
(Merged current main, before the squash & merge.)

@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #306 (e125019) into main (ce9f034) will increase coverage by 0%.
The diff coverage is 100%.

@@         Coverage Diff         @@
##           main   #306   +/-   ##
===================================
  Coverage    96%    96%           
===================================
  Files        91     91           
  Lines      3184   3186    +2     
===================================
+ Hits       3068   3070    +2     
  Misses      116    116           
Impacted Files Coverage Δ
...rtFormat/Extensions/PluralLocalizationFormatter.cs 89% <100%> (ø)
src/SmartFormat/SmartFormatter.cs 97% <0%> (ø)
src/SmartFormat/Core/Parsing/Format.cs 96% <0%> (ø)
src/SmartFormat/Core/Sources/Source.cs 100% <0%> (ø)
src/SmartFormat/Extensions/ListFormatter.cs 98% <0%> (+<1%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -113,7 +113,7 @@ public bool TryEvaluateFormat(IFormattingInfo formattingInfo)
// We can format numbers, and IEnumerables. For IEnumerables we look at the number of items
// in the collection: this means the user can e.g. use the same parameter for both plural and list, for example
// 'Smart.Format("The following {0:plural:person is|people are} impressed: {0:list:{}|, |, and}", new[] { "bob", "alice" });'
if (current is IConvertible convertible)
if (current is IConvertible convertible && current is not bool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess, I oversaw an my mobile, that the test is already there.
Thanks a lot for submitting the fix.
(Merged current main, before the squash & merge.)

@axunonb axunonb merged commit 24510e8 into axuno:main Aug 21, 2022
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.

None yet

2 participants