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

Make Option<T>.ToString return string value of Some or blank #17

Open
atifaziz opened this issue Dec 22, 2016 · 2 comments
Open

Make Option<T>.ToString return string value of Some or blank #17

atifaziz opened this issue Dec 22, 2016 · 2 comments

Comments

@atifaziz
Copy link
Contributor

atifaziz commented Dec 22, 2016

Option<T>.ToString() is currently implemented to return a string representation of Option<T>'s state when ideally it should simply return a blank when None or Some(null) and the string representation of the value of Some otherwise. In other words:

        public override string ToString() =>
            hasValue && value != null ? value.ToString() : string.Empty;

Doing so will go a long way to help Option<T> to be easily substituted in code for Nullable<T> and references where one desires. The trouble with the current implementation is that any refactoring towards Option<T> will require one to thoroughly review all code where string formatting is used. For example, suppose the following code:

var d = (DateTime?) new DateTime(2010, 12, 31)
var s = string.Format(new CultureInfo("de-DE"), "{0:D}", d);
Console.WriteLine(s);

This prints:

Freitag, 31. Dezember 2010

However, if you use Option<T> instead:

var d = Option.Some(new DateTime(2010, 12, 31));
var s = string.Format(new CultureInfo("de-DE"), "{0:D}", d);
Console.WriteLine(s);

then output changes to (depending on the thread's default culture) something along the lines of:

Some(31/12/2010 00:00:00)

If you have unit tests then you can catch this sort of issue otherwise your luck will be your friend or your enemy. Either way, the fix looks ugly:

var d = Option.Some(new DateTime(2010, 12, 31));
var s = string.Format(new CultureInfo("de-DE"), "{0:D}",
                      d.Match(v => v, () => default(DateTime?)));
Console.WriteLine(s);

For debugging purposes, one can still rely on DebuggerDisplayAttribute to display the string representation as it is today.

@nlkl
Copy link
Owner

nlkl commented Dec 23, 2016

Hi,

I think consistency with nullable is a good idea, and a very reasonable expectation (in retrospect, it seems like the obvious thing to do).

I consider the suggestion approved. It is, however, a breaking change, so I will designate this for version 4.0.0.

Thanks a lot for the input!

@nlkl nlkl added this to the Optional 4.0.0 milestone Dec 23, 2016
atifaziz added a commit to atifaziz/Optional that referenced this issue Dec 23, 2016
This renders Option<T>.ToString() consistent with Nullable<T>.ToString() and easier to substitute for.

Closes nlkl#17
@nlkl nlkl removed this from the Optional 4.0.0 milestone Aug 13, 2017
@mathiasbaert
Copy link

Hi @nlkl
I'm wondering why this change was removed again from 4.0.0
Do you no longer consider this to be the preferred behavior
or were there unforeseen problems?

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