Discuss: Global Config design

[clairernovotny]

Today Humanizer uses a few static configuration options to control formatting/output behavior of some of its methods.

Aside from this design making it harder to run unit tests in parallel, it does place certain restrictions on the library usage - namely that you couldn't have different options depending on the context as they're static.

This may or may not be a real issue but I wanted to start a discussion around it here.

Thoughts?

[aloisdg]

I vote for a global config file. I think more flexibility is good thing.

[tiesmaster]

Maybe this goes completely against the design of Humanizer, but I'd vote for neither one: don't use static, or configuration, but keep the flexibility with overloads, or constructors when instantiating types.

Why? Although there are other NuGet packages that use static methods, and properties to configure it (like AutoMapper), for me it feels like an old-school approach of providing an API for you as a developer: since they can be accessed from anywhere, they also behave as globals (you never know when, and how things are configured). My experience with that is that it gives a lot of headaches.

These days, dependency injection is becoming the defacto standard, and there are good ways of making things global in that, if you need that when you want to set something global. The same applies to configuration, that doesn't belong in something like a class library like Humanizer.

[mexx]

The current design of the API is based upon extension methods, those are static.
For the case of accessibility of the functionality it's OK to have them static. The only thing they should not rely on any mutable shared state, as this brings in the problems mentioned in the issue description.

IMHO we should drop the current support for configuration.
The configuration of operations should be provided as an instance directly to the method performing the operation. For convenience we should provide overload for the common scenarios where the configuration would be populated with some reasonable defaults.

We in generally already take this route, let's take it to the end.

[hazzik]

I agree with @mexx.

As I understand actions needed to be performed are:

1. to make Configurator class internal
2. to add overloads where we use Configurator exposed properties.
3. [optional] to add static enum-like strategies for Configurator exposed properties (same as truncator)

[clairernovotny]

@hazzik @mexx Sounds good to me - I'm not a huge fan of static variables so anywhere we can get rid of them is a win to me.

[tiesmaster]

👍

[MehdiK]

The reason I added the static configurator was to allow global configuration without enforcing devs to have to pass the config in every single method. Optional defaults work as long as you want the default value otherwise you will have to pass your override to all method calls. I am also not a big fan of methods with long argument lists which typically happens with a lot of optional params (Perhaps we could pass configurator options/hashes to methods to avoid an ever-growing API surface). All that said, I agree with the pain of shared static state.

[mexx]

I also not a fan of methods with lots of parameters or overloads.
My suggestion was to introduce methods that accept one configuration object of type specific to the given functionality.
I think some code explains it better than thousand words.
I pick timespan.Humanize() as this was the starting point of the discussion.

public static string Humanize(this TimeSpan timeSpan, TimespanConfiguration configuration)
{
    IEnumerable<string> timeParts = CreateTheTimePartsWithUperAndLowerLimits(timeSpan, configuration.Culture, configuration.MaxUnit, configuration.MinUnit);
    timeParts = SetPrecisionOfTimeSpan(timeParts, configuration.Precision, configuration.CountEmptyUnits);

    return ConcatenateTimeSpanParts(timeParts, configuration);
}

public class TimespanConfiguration {
    public CultureInfo Culture { get; set; }
    public int Precision { get; set; }
    public TimeUnit MinUnit { get; set; }
    public TimeUnit MaxUnit { get; set; }
    public bool CountEmptyUnits { get; set; }

    public TimespanConfiguration() {
        CultureInfo = null;
        Precision = 1;
        MinUnit = TimeUnit.Millisecond;
        MaxUnit = TimeUnit.Week;
        CountEmptyUnits = false;
    }
}

With this in place, we can now write

timespan.Humanize(new TimespanConfiguration { CountEmptyUnits = true });

or the currently present method would look like that

public static string Humanize(this TimeSpan timeSpan, int precision = 1, CultureInfo culture = null, TimeUnit maxUnit = TimeUnit.Week, TimeUnit minUnit = TimeUnit.Millisecond)
{
    return Humanize(timeSpan,
                       new TimespanConfiguration {
                                 Precision = precision,
                                 Culture = culture,
                                 MinUnit = minUnit,
                                 MaxUnit = maxUnit });
}
public static string Humanize(this TimeSpan timeSpan, int precision, bool countEmptyUnits, CultureInfo culture = null, TimeUnit maxUnit = TimeUnit.Week, TimeUnit minUnit = TimeUnit.Millisecond)
{
    return Humanize(timeSpan,
                       new TimespanConfiguration {
                                 Precision = precision,
                                 CountEmptyUnits = countEmptyUnits,
                                 Culture = culture,
                                 MinUnit = minUnit,
                                 MaxUnit = maxUnit });
}

Thoughts?

[MehdiK]

The configurator in Seleno worked very nicely for us. Here is the caller API and the configurator and a simple example. It's very similar to your suggestion @mexx but you won't have to initiate the configuration object.

[ErikSchierboom]

I really like the idea of separate configuration classes that contain the configuration options. Although @MehdiK's suggestion looks not that bad, I think introducing a lambda to configure an instance is not the most user-friendly way to go.

[mexx]

In both cases there is a need for an instance to hold the configuration options. In @MehdiK's suggestion this object is created inside the method and passed out to allow customization, in my case the object is created outside.

I would go even further and say, the parameter should be an interface. That way we can decouple the implementation even more, as a potential user could provide a complete own implementation of the configuration options, which delivers different results in dependency of a context. Thoughts?

[ErikSchierboom]

An interface would provide decoupling, but at the expense of users not being able to discover easily which class to create that implements that interface.

[mexx]

Right, maybe a good naming convention can help?

[ErikSchierboom]

That certainly would help. But when would you imagine someone creating their own configuration options class?

[mexx]

As I said, when the configuration should depend on some state. I could implement the needed interfaces to hide the access to this state in the implementation. Currently to support this scenario you would have to provide a factory for the configuration to the methods that call Humanizer.

Further I have one more point why interfaces are a better fit.
We want to reuse the Collection formatting for the TimeSpan formatting and maybe some other formatting also. If we go the route with the interfaces we can simple compose multiple configurations by inheriting multiple interfaces, and pass the instance down to the utility function used inside. We can't achieve this by using classes.

Some code example:

interface IConfigureCulture {
   CultureInfo CultureInfo { get; }
}

interface IConfigureCollectionSeparator : IConfigureCulture {
   string CollectionSeparator { get; }
}

interface IConfigureTimeUnits : IConfigureCulture {
    TimeUnit MinUnit { get; }
    TimeUnit MaxUnit { get; }
}

interface IConfigureUnitPrecision {
    int Precision { get; }
    bool CountEmptyUnits { get; }
}

interface IConfigureTimeSpan :
    IConfigureTimeUnits, IConfigureUnitPrecision, IConfigureCollectionSeparator {}

public static string Humanize(this TimeSpan timeSpan, IConfigureTimeSpan configuration) {
    IEnumerable<string> timeParts =
        CreateTheTimePartsWithUperAndLowerLimits(timeSpan, configuration as IConfigureTimeUnits);
    timeParts = SetPrecisionOfTimeSpan(timeParts, configuration as IConfigureUnitPrecision);

    return ConcatenateTimeSpanParts(timeParts, configuration as IConfigureCollectionSeparator);
}

The casts aren't necessary, I've included them for clarification of my intent.

[ErikSchierboom]

That is true. I understand what you're going for now. Looks fine.

[hazzik]

We can hide this under the system interface IFormatProvider, probably
On Wed, 20 Jan 2016 at 1:57 AM Erik Schierboom notifications@github.com
wrote:

That is true. I understand what you're going for now. Looks fine.

—
Reply to this email directly or view it on GitHub
#508 (comment).

[mexx]

@hazzik Could you elaborate more on how IFormatProvider can help?

[hazzik]

@mexx forget, we can not. I was thinking that we can implement custom IFormatProviders for built-in types, but it turns out, that we can not.

[kentcb]

Global-only configuration is a huge pain point for me.

I didn't see this explicitly mentioned, but the way I see this could work is that locally provided config takes precedence over global. That way one can set global defaults and override those where necessary.

Pseduo-code:

Configurator.DateTimeConfiguration = ...;

var now = DateTime.Now;
var useGlobal = now.Humanize();
var useLocal = now.Humanize(new DateTimeConfiguration(...));

The actual API could expose the one method with a default:

public static string Humanize(this DateTime @this, DateTimeConfiguration configuration = null)
{
    configuration = configuration ?? Configurator.DateTimeConfiguration;
}

[mexx]

@kentcb Thanks for this input, also the overloads with the additional parameters can utilize this technique. We wouldn't even break the current state of the configuration, at least it's my hope. What is beneficial to projects those are happy with the global configuration approach.