Unable to test for a config key existence #162

Closed
Haacked opened this Issue Jun 1, 2012 · 15 comments

Comments

Projects
None yet
4 participants
Contributor

Haacked commented Jun 1, 2012

I'd like to be able to tell the difference between a case where a config setting is missing vs it's value is null. For example, right now to test if some setting is missing, I have to do this:

var missingToken = Guid.NewGuid().ToString();
if (repository.Config.Get("SETTINGNAME", missingToken) == missingToken)
{
    Console.Write("The key 'SETTINGNAME' is missing");
}

It'd be nice if I could simply do this:

repository.Config.ContainsKey("SETTINGNAME");

Or even better, make the operation atomic.:

repository.Config.TryGet("SETTINGNAME"); 

This would return value with 2 properties, bool Exists and string Value.

What do you think?

Also, while I'm on the subject, how do I determine where the setting came from?

Member

nulltoken commented Jun 1, 2012

I agree. This would definitively make sense.

Provided I come to an agreement with @carlosmn on something like this, I was thinking about changing the Get API to something like this:

ConfigEntry<T>  entry = repository.Config.Get<T>(string key, ConfigurationLevel level = ConfigurationLevel.Local)

Which would return null for an non existing key.

where ConfigEntry would be more or less like this

public class ConfigEntry<T>
{
    ConfigurationStore ConfigurationStore { get; }
    T Value { get; }
}

ConfigurationStore being an enum { Local, Global, System }

Would that fit your need?

Contributor

Haacked commented Jun 1, 2012

@nulltoken Yeah, sounds great!

Why not leave it untyped and just have extension methods to perform the appropriate casting?

Contributor

Haacked commented Jun 1, 2012

Yeah, that makes implementing the interface easier and more flexible. :)

Member

nulltoken commented Jun 2, 2012

@davidfowl

Why not leave it untyped and just have extension methods to perform the appropriate casting?

I like the idea! So we'd go with something like

public class Configuration : IDisposable
{
    // Stuff

    public ConfigurationEntry Get(string key, ConfigurationLevel level = ConfigurationLevel.Local)
    {
    }

    // More stuff
}

public class ConfigurationEntry<T>
{
    public ConfigurationStore ConfigurationStore { get; private set; }
    public T Value { get; private set; }
}

public class ConfigurationEntry : ConfigurationEntry<string>
{
}

@nulltoken Why does the configuration object returned need a back pointer to where it came from? What are the scenarios for that? Most configuration APIs have 2 things:

  1. Automatic fallback logic when the scope isn't specified
  2. The ability to specify a specific scope.
public interface IConfiguration
{
    public string Get(string key, ConfigurationLevel level =  ConfigurationLevel.Local) 
    {
    }
}

public static class ConfigurationExtensions
{
    public static T GetValue<T>(this IConfiguration config, string key, ConfigurationLevel level = ConfigurationLevel.Local)
    {
        string value = config.Get(key, level);
        return (T)Convert.ChangeType(value, typeof(T));
    }
}

I guess I'm not understanding why you need ConfigEntry at all.

Member

nulltoken commented Jun 3, 2012

@davidfowl

Why does the configuration object returned need a back pointer to where it came from?
I guess I'm not understanding why you need ConfigEntry at all.

Easy and short answer: Because I wasn't clear enough ;-) Let me try once again.

This request originates from a discussion that happened in issue libgit2/libgit2sharp#161

  • Current Get() implementation searches in the local config, then if nothing has been found, tries to find something in the global one, then the system one. This is standard Git probing behavior, we should keep on supporting it and this should be the standard behavior. The proposed API should as well expose an overload without a ConfigurationLevel.
  • However, it might be useful to also be able to specify a scope into which the Get() should operate. The ConfigurationLevel is an enum to help with this. It might even become a Flags if we're willing to select more than one target config file.
  • When "probing for a config key", one might willing to know where does the value comes from as the same entry may simultaneously exist in different files. This is not supported by Git and this "feels" like missing to me.
  • When a key doesn't exist at all, null should be returned rather than throwing an Exception.

As it's not possible to define overloads that only defer by the type of the output parameter, the ConfigEntry was an attempt to try and tackle all those requirements at once (I'm really not a fan of out parameters).

Note: I agree that the casting shouldn't be more complex than your proposal. However, we'll have to support "rich" bools (1/0, yes/no, on/off, true/false) as git_config_get_bool() already does.

Contributor

Haacked commented Jun 3, 2012

There are cases where we want to know the value and where we got it from. For example, in some cases we can fix up a setting. But if we don't know where it came from, we now have to bake in knowledge in our code on how Git does config resolution.

It's not a terrible thing since it's unlikely to ever change, but doesn't seem right to duplicate that logic in our code.

This seems like a solved problem to me and one where there's much prior art to learn from:

  1. System.Configuration (http://msdn.microsoft.com/en-us/library/system.configuration.configurationmanager.aspx)
  2. GetEnvironmentVariable (http://msdn.microsoft.com/en-us/library/system.environment.getenvironmentvariable.aspx)
  3. TryGetValue (http://msdn.microsoft.com/en-us/library/bb347013.aspx)
Contributor

Haacked commented Jun 3, 2012

All kind of shitty examples that require you to understand the inner workings of how fallback works. Out parameters suck. ;)

To me it's all about scenarios, if you do have a concrete reason for wanting to know where a config value came from then I guess it makes sense, but it doesn't feel right to me.

Contributor

Haacked commented Jun 3, 2012

Yeah, I'm surprised we don't see this pattern more in .NET. But look around at other configuration systems in other platforms. Do they follow the same pattern?

I think in part, the reason System.Configuration doesn't care where the value comes from is it's a read-only configuration API.

In our case, we have cases where somehow, people have the wrong proxy config:

[http]
    proxy=

So this returns a value of null. What we need to do is delete that value, but we need to know where it came from to do that. Seems wrong for me to query each level in the proper order to figure it out.

What if it exists in multiple places? Do you want to delete all? How much detail do you need from the config api? Is the store the a pointer to the actual config object?

Is this what that would look like?

var entry = config.Get("http.proxy");
if(entry.ConfigurationStore.Level == ConfigurationLevel.Local) {
    config.Delete("http.proxy");
}
Contributor

Haacked commented Jun 4, 2012

We only want to delete it if it's causing a problem. So the code would be something like this under the new system:

var entry = config.Get("http.proxy");
while (entry != null && entry.Value == null) {
    config.Delete("http.proxy", entry.ConfigurationStore.Level);
    entry = config.Get("http.proxy");
}

Though I'd probably not use that exact code since I'm paranoid about infinite loops. ;)

For example, if you had proxy= for local, proxy=something for global, and proxy= for system, we wouldn't touch the system one because after we deleted local, we're good. We don't want to dig deeper than we have to.

Phil

Since you want to allow auto fallback and explicit search, Why don't you look at making ConfigurationLevel [Flags] based and change the default value to ConfigurationLevel.Any

public class Configuration : IDisposable
{
    // Stuff

    public ConfigurationEntry Get(string key, ConfigurationLevel level = ConfigurationLevel.Any)
    {
        // Implement query in fallback order where flags are set
        if ((level & ConfigurationLevel.Local) == ConfigurationLevel.Local) { ... }
        if ((level & ConfigurationLevel.Global) == ConfigurationLevel.Global) { ... }
        if ((level & ConfigurationLevel.System) == ConfigurationLevel.System) { ... }
    }

    // More stuff
}

[Flags]
public enum ConfigurationLevel {
    Local = 1,
    Global = 2,
    System = 4,
    Any = Local | Global | Any
}

yorah added a commit to yorah/libgit2sharp that referenced this issue Nov 9, 2012

yorah added a commit to yorah/libgit2sharp that referenced this issue Nov 23, 2012

@nulltoken nulltoken closed this in bbf0b7f Dec 5, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment