Skip to content

Fix thousands separator#11

Merged
andersnm merged 2 commits intoandersnm:masterfrom
ClosedXML:thousands-separator
Nov 23, 2017
Merged

Fix thousands separator#11
andersnm merged 2 commits intoandersnm:masterfrom
ClosedXML:thousands-separator

Conversation

@igitur
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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