Add option to customize list of time indicators of humanized timespans

[tiesmaster]

Probably this title needs some explanation ;) When you humanize a timespan, like this:

        const int ONE_WEEK_ONE_DAY = 8;
        var oneWeekOneDayTimeSpan = TimeSpan.FromDays(ONE_WEEK_ONE_DAY);
        var result = oneWeekOneDayTimeSpan.Humanize(2);

then you get "1 week, 1 day". However, what I want is to have the option to override the concatenation of the strings, to have an ", and", instead of only a comma between the second to last, and last item, making the output this:

1 week, and 1 day

AFAICT, you cannot do this now with Humanizer, and couldn't find an issue for this. I'd suggest adding a separator argument to the call, similar to how you can specify that on the Humanize call for collections. However, I'm not sure if you want another overload for this, besides the other two.

I'm happy to make a PR, if my proposal is accepted, and we can find a good way to add it to the API.

[mexx]

The current implementation of TimeSpan humanization doesn't utilize the functionality of Collection humanization. IMHO this would be the correct way to implement this feature.

[tiesmaster]

Ok, cool ;) Concerning the implementation: adding a new overload is fine?

[mexx]

Maybe already the use of the Collection humanization would be sufficient, as for en it uses the new OxfordStyleCollectionFormatter("and").
But if you have to provide another separator, then the only option would be to add a new overload.

[clairernovotny]

Tagging as vNext unless this can be completed in the next couple of weeks.

[ErikSchierboom]

@mexx I'm ooking into this. If I change the ConcatenateTimeSpanParts() method to use Configurator.CollectionFormatter.Humanize(timeSpanParts);, I get the desired ", and" behavior. However, the existing tests in the TimeSpanWithMinTimeUnit now fail as instead of outputting "2 seconds, 500 milliseconds", it now outputs "2 seconds and 500 milliseconds". Should I create an overload to not break any existing code?

[tiesmaster]

👍

Thanks! I started with this just after I created the issue, but I was unable to built the solution. And although, @ErikSchierboom commit shows it's a very simple change, I did wanted to be able to test my own work ;)

FWIW, I remember that I did open the non-Dnx solution, nevertheless, I had build errors concerning Dnx. It might be useful to put in some notes in the CONTRIBUTING.md file, if there are special requirements for the solution (and in case I didn't do some stupid things, I'll see if I can reproduce the errors).

[clairernovotny]

Hi @tiesmaster, what sort of build errors are you seeing? The non-Dnx solution should build cleanly; the Dnx solution really only exists to host the dnx unit tests for now.

[mexx]

@onovotny @MehdiK @hazzik What do you guys think about this question from @ErikSchierboom:

Should I create an overload to not break any existing code?

Actually we only change the output, however this might not be desired by all of the users of Humanizer.

1. We can provide an optional parameter to control whether the culture comes into play, defaulting to the old behavior of cause ;)
2. We put this into v2.0 and announce this as kind of breaking change.

[tiesmaster]

@onovotny I tried reproducing it just now, and I got specifically errors on the Uwp projects (though I do have the Visual Studio Tools for Universal Windows Apps installed, but I am still on Windows 7). After disabling those projects, I was able to run the unit tests using the default runner of VS. However, both ReSharper and NCrunch runners don't work :( For NCrunch, I'm getting a TargetInvocationException. I can post the details in the gitter chat, if you're interested?

@mexx FWIW, I know that my designers here wouldn't be too happy if a minor NuGet update of Humanizer would break our visual applications. My intention would have been to just add an overload to keep it backwards compatible, but making this the default in a major update would make me even happier ;)

[MehdiK]

Minor bumps shouldn't change the output as @tiesmaster mentioned; but since we're making a major release, a breaking change like this is understandable as long as there's a way for users to get the old behaviors (through the optional param).

I think this kind of settings/configs are more global than just method level. So perhaps we can add the separator as a global config and just leave it as is, allowing users to specify a different value. This way we are not introducing a breaking change while users can still define what they want as a separator. If more granular control is required we could then add the optional param defaulting to the old value! Thoughts?

[tiesmaster]

👍

[clairernovotny]

My only concern overall is with the statics of global config. Aside from making it harder to run all of the tests in parallel, it could create limits if someone wants it configured a certain way in one context but not another.

I've started a new discussion around this topic in #508

[ErikSchierboom]

Pending the global config discussion, I have updated my code and submitted a PR: #511 In this code, I added a collectionSeparator parameter to the Humanize method that is set to null. If null is passed as this parameter, the "and" string-based collection formatter is used. If not null, that value is used as the separator.

I could then fix the failing unit tests by passing collectionSeparator: ", " to it. This indeed fixed the tests.

I also considered making the collectionSeparator parameter not be a string but a ICollectionFormatter instance. However, I was not sure that would be the way to go. Is it?

[mexx]

I've put a comment on the PR.

About the ICollectionFormatter, I think for now it's fine that collectionSeparator is simply a string, especially with regard of the discussion of #508.

[ErikSchierboom]

Okay, great. I've updated the PR according to your comment.

[tiesmaster]

Awesome to see that this has been merged!! 🎉 Thanks!

AFAICT, this is not yet publicly available on NuGet. Any idea when it will be?