axuno / SmartFormat

A lightweight text templating library written in C# which can be a drop-in replacement for string.Format
Other
1.1k stars 105 forks source link

Choose not working as expected #46

Closed msancho closed 9 years ago

msancho commented 9 years ago

Hi,

I was playing with Choose option and I noticed that the output was not the desired one and I tried then the examples in https://github.com/scottrippey/SmartFormat.NET/wiki/Choose

This is some code to exemplify the issue:

var number = 1;
var boolean = true;
var c1 = Smart.Format("The number is {0:choose(1,2,3):one|two|three|other}", number);
var c2 = Smart.Format("The number is {0:choose(1,2,3):one|two|three|other}", 2);
var c3 = Smart.Format("The number is {0:choose(true,false):bigger|smaller}", true);
var c4 = Smart.Format("The number is {0:choose(true,false):bigger|smaller}", "true");
var c5 = Smart.Format("The number is {0:choose(true,false):bigger|smaller}", boolean);

c1, c2 and c3 are always: The number is c4 and c5 are always: The number is smaller

Version used is

<package id="SmartFormat.NET" version="1.5.0.0" targetFramework="net452" />

I hope that information helps, Thanks!

scottrippey commented 9 years ago

Thanks. You're absolutely right, I see the same issue. https://dotnetfiddle.net/5gJOmU
I'm perplexed why this would be -- we've got tons of unit tests. I'll investigate.

msancho commented 9 years ago

Thanks for checking and congrats for the library is very helpfull!

If you need further help, let me know!

scottrippey commented 9 years ago

Well, this is embarrassing. I documented the feature incorrectly. I went back-and-forth a few times, but finally decided to use | instead of , (to match the choices). And apparently, I forgot that when I started writing the documentation. https://dotnetfiddle.net/KiQQpQ

The correct syntax is:

Smart.Format("The number is {0:choose(1|2|3):one|two|three|other}", number);

I'll update the documentation.

scottrippey commented 9 years ago

Oh, and I found something else interesting... Apparently, true.ToString() == "True" ... so, you must write

Smart.Format("The number is {0:choose(True|False):bigger|smaller}", true);

One way around this is to enable the CaseInsensitive option. However, I'll consider creating a fix for this in the next version.

scottrippey commented 9 years ago

Quick question for you, though ... what do you think about using | instead of ,?
In the "output" section, I consistently use | for all plugins, and I'm happy with this choice, because I'll probably never actually use a | in my output. So I'll never need to escape it, which would be really messy if I used ,. But in this "choose(options)" section, it already looks like a function, which is why the , made sense. However, I do like the parallel between the "option|option" and "output|output", so I'm torn.

msancho commented 9 years ago

It is working now! :)

I think that is better to use parallel as separator in both and to me looks more consistent and as you say, is usually comma what you use as a separator for a list (although I tried to use | as list separator symbol just 20 minutes ago, just for fun).

Thanks!