Skip to content

Conversation

igitur
Copy link
Contributor

@igitur igitur commented Nov 19, 2017

Found a case that isn't handled correctly with regards to thousands separator. Hope you agree with this fix.

public void TestThousandSeparator()
{
    var actual = Format(1469.07, "0,000,000.00", CultureInfo.InvariantCulture);
    Assert.AreEqual("0,001,469.07", actual);
}

@andersnm
Copy link
Owner

Thanks, nice find! The fix looks correct, but a minor nitpick: I would prefer without the Token.IsSignificantPlaceholder(string token) helper, and replace with c == "0" directly in the modified if-clause. Could you do that too please?

Because there's a couple other places testing against "0", "?", "#" etc for similar purposes, and their meanings are kind of assumed implicitly from the context. It's definitely a good idea to replace these hardcoded strings with constants globally and consistently at some point!

@igitur
Copy link
Contributor Author

igitur commented Nov 23, 2017

Changes made and rebased.

@igitur igitur force-pushed the thousands-separator branch from 7288c8b to d247f56 Compare November 23, 2017 10:49
@andersnm andersnm merged commit 3c9a2a6 into andersnm:master Nov 23, 2017
@igitur igitur deleted the thousands-separator branch November 28, 2017 08:41
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