special treatment of '' in Component.config.get()

[thomas.moschny@…]

Component.config.get("mysection", "myoption", None) returns '' (the empty string) in both cases, if myoption in [mysection] is empty and if myoption is not present at all.

The third argument is supposed to be the default value, so the code shown above should return None in case the option is not present, and its value if it's present (including '' when the option is present but empty).

There was at least on additional voice on #monotone calling this a bug.

[thomas.moschny@…]

Replying to thomas.moschny@gmx.de:

There was at least on additional voice on #monotone calling this a bug.

I wanted to say: one additional voice on #trac, of course.

[osimons]

I have a problem seeing the need to differentiate here. If you want to check if a certain option exists in config, you would use something like 'myoption' in config.options('mysection') giving True or False. With the config/option registration system, also note that what exists in the config object may not be identical to what is written in trac.ini file(s).

Most importantly though is the fact that the config.get() method is a fetch-string-from-config call. As opposed to getint(), getlist(), getbool(). It should always return a string to avoid all kinds of issues when using the settings - lots of extra code and checks to make sure we don't perform string operations on None objects.

My vote is for closing as 'wontfix'.

[Christian Boos]

I think there's at least the API or the API doc to fix, here. I agree with Thomas that the behavior he described is what one would normally expect. It's also what happens in most cases except when the default value is None, because of an explicit check for that value (see source:/trunk/trac/config.py@6242#L253​).

So I suggest doing the following change:

• change the default value for keyword argument default to be '' instead of None
• remove the 2 lines 253 and 254.

That way, one would always get a string from config.get() as osimons wrote above, except when explicitly asking for something else, like in config.get('a', 'b', None). In the latter case, it's plain clear you should be prepared to deal with None.

[thomas.moschny@…]

It is about consistency. The last argument for the config.get() method is what is returned in case the option is not present, and it's not ok to return then. In fact, following this argumentation would render this last argument useless.

There is definitively a difference between specifying an empty string for an option in trac.ini or not specifying that option at all. I don't see why I should introduce an extra boolean option (and some logic evaluating it), when this could also be handled cleanly withing trac.

If some client doesn't want to check for None, he can supply '' as last argument, or nothing if that'd be the default value for the argument.

[thomas.moschny@…]

Whoops, screewed up formatting, sorry.

[osimons]

Replying to thomas.moschny@gmx.de:

There is definitively a difference between specifying an empty string for an option in trac.ini or not specifying that option at all.

Kind of. But not always as what you are interfacing with is the Trac config abstration layer, and not ConfigParser directly (which would obviously have raised an exception when trying to fetch a non-existing option). If you are requesting an option that isn't set in trac.ini, but exists in Option registry, you are likely to actually get a value - and not '' or None. Remove a setting like [project] icon from the trac.ini file, and make the get() call. It will return common/trac.ico no matter what you set as default. Should it do so if you pass a default? Could argue that you are equally well prepared to handle your own default?

Anyway, also checked the getint() behavior. It also has a default of None, returning that when no option exists (or it exists in file as empty…). If you specifically ask for 0 as default, then you get that. getlist()however returns [], while getbool() returns False. All the other 3 return the default from call when no option exists. String should then naturally do the same.

Here is then my new conclusion:

• I agree; if neither in registry nor in file, return empty string as default - or whatever is passed in as default value.
• I actually think getint() should be changed to default to 0 so that all 4 methods return default values of their base type. i = int() sets i to 0 (zero)…

[thomas.moschny@…]

Replying to osimons:

Kind of. But not always as what you are interfacing with is the Trac config abstration layer, and not ConfigParser directly (which would obviously have raised an exception when trying to fetch a non-existing option). If you are requesting an option that isn't set in trac.ini, but exists in Option registry, you are likely to actually get a value - and not '' or None.

Ok, this is a completely different topic I was not aware of. If some option is not present in trac.ini, but nevertheless has a value inside the trac config abstraction layer, then it is perfectly ok to return that value and not a default given by the caller.

Anyway, also checked the getint() behavior. It also has a default of None, returning that when no option exists (or it exists in file as empty…). If you specifically ask for 0 as default, then you get that. getlist()however returns [], while getbool() returns False. All the other 3 return the default from call when no option exists. String should then naturally do the same.

Maybe we can't count on current getint() or getlist() behavior, because they use get() in turn ;)

Nevertheless, all getX() methods have to be looked at for consistency.

Here is then my new conclusion:

• I agree; if neither in registry nor in file, return empty string as default - or whatever is passed in as default value.

The latter is what this ticket is about. So we have a consensus here.

• I actually think getint() should be changed to default to 0 so that all 4 methods return default values of their base type. i = int() sets i to 0 (zero)…

Additionally, getlist() must be fixed, and also should return the default passed by the caller. As it currently looks like, it could throw a TypeError after the change proposed for get(), and passed default=None because None can't be converted to a list in source:/trunk/trac/config.py@6242#L294​.

[osimons]

Patch that makes defaults more clean and consistent - but not quite like the ticket requested.

[osimons]

Regarding t4977a-config_defaults-r6251.diff​…:

Defaults on options come at a trade-off. It cannot solve too many things without becoming very complex.

Problem 1: The default= keyword argument gets passed around a lot. So changing it on one entry means that effectively that is what all the derived uses will actually get as well. The patch therefore makes empty string the default for consistency - although probably not needed as this is the essence of the larger patch (not 100% as it did not pass 2 tests - string version passed all):

trac/config.py

@@ -250 +250 @@
                 value = option.default or default
             else:
                 value = default
-        if value is None:
-            return ''
-        return to_unicode(value)
+        if not value:
+            return default or u''
+        if isinstance(value, basestring):
+            return to_unicode(value)
+        else:
+            return value
 
     def getbool(self, name, default=None):
         """Return the value of the specified option as boolean.
@@ -272 +275 @@
         `ConfigurationError` exception is raised.
         """
         value = self.get(name, default)
-        if value == '':
-            return default
+        if not value:
+            return default or 0
         try:
             return int(value)
         except ValueError:
@@ -288 +291 @@
         from the list.
         """
         value = self.get(name, default)
+        if not value:
+            return default or []
         if isinstance(value, basestring):
             items = [item.strip() for item in value.split(sep)]
         else:

trac/tests/config.py

@@ -68 +68 @@
     def test_default_int(self):
         config = self._read()
         self.assertRaises(ConfigurationError, config.getint, 'a', 'option', 'b')
-        self.assertEquals(None, config.getint('a', 'option'))
+        self.assertEquals(0, config.getint('a', 'option'))
         self.assertEquals(1, config.getint('a', 'option', '1'))
         self.assertEquals(1, config.getint('a', 'option', 1))

Problem 2: What to return when no default is specified. Here is a brief comparison of current trunk vs. patch:

call	current trunk	patch
config.get('non', 'existing')	''	u''
config.getint('non', 'existing')	None	0
config.getbool('non', 'existing')	False	False
config.getlist('non', 'existing')	[]	[]

Problem 3: It is a good thing to return intuitive and useful defaults for the various types when no values exists, or no particular default is passed. However, doing that, and at the same time for getint() for instance having to distinquish between empty string, False, 0, None or whatever, and deciding what to return for essentially no value, becomes a bit too much for very little. Here is the status for passing default=None to the call:

call	current trunk	patch
config.get('non', 'existing', None)	''	None (or u'')
config.getint('non', 'existing', None)	None	0
config.getbool('non', 'existing', None)	False	False
config.getlist('non', 'existing', None)	[]	[]

One change - get() with default=None actually returns None. However, the inline small adaptation returns u'' that feels better…

Problem 4: What actually happens when instead of None, you pass in meaningful defaults to the various calls? Here is one last rundown:

call	current trunk	patch
config.get('non', 'existing', 'hello')	u'hello'	u'hello'
config.getint('non', 'existing', 42)	42	42
config.getint('non', 'existing', '42')	42	42
config.getbool('non', 'existing', True)	True	True
config.getbool('non', 'existing', 'True')	True	True
config.getlist('non', 'existing', [u'working', u'well'])	[u"[u'working'", u"u'well']"]	[u'working', u'well']
config.getlist('non', 'existing', 'working, well')	[u'working', u'well']	[u'working', u'well']

I think the consistency of handling no defaults + meaningful defaults for the various access methods, is really what matters to the option handling. I also have a patch that returns None for all when None is passed in, but that then breaks the meaningful defaults for various data types. And passing other variations of no value (like False) complicates further.

⇒ I think the ideas from this patch is a good thing. And, having reviewed this quite well now, I have ended back on my original feeling: Even a None default for a string get() should return empty string. That would make all use-cases consistent.

(Documentation changes not done - would also need to be updated if this is changing)

(Much too late now - hope this all makes sense…)

[Alec Thomas]

LGTM.

Only thing we should perhaps consider is whether to raise a KeyError when a default isn't provided and the key doesn't exist. This could be a bit drastic, but it could also pick up potential bugs. *shrug*

[Alec Thomas]

Forgot to mention: I think the patch improves the config module by making the behaviour more consistent. Specifically getlist() with a list default returning a list, and getint(..., default=None) returning an int as the other get* methods do.

[Christian Boos]

I'm OK with the results shown in problem 2 and problem 4 in comment:8. However I differ on the problem 3, as I think we should guarantee the following behavior: config.get*('non', 'existing', X) is X. But we should also have config.get*('existing', 'but_empty', X) == default, where default is the appropriate default for the type. I think we should make that default visible in the method signature, e.g. def getint(section, entry, default=0), as that would be cleaner.

I'll try to come up with a variant of the patch implementing the above.

[thomas.moschny@…]

Replying to osimons:

Even a None default for a string get() should return empty string. That would make all use-cases consistent.

It is not clear to me why you insist on this idea. When the caller passes None in as default, he surely signals that he is able to handle None as a return value. Why should the method "know it better" and return an empty string instead? This makes passing None as default essentially useless, and thus makes it impossible to distinguish between an empty and a non-existant option, as I said earlier. (And I'd consider introducing an extra boolean option that signals the validity of the option in question an ugly workaround.)

Lists are admittedly somewhat special, because in many use cases it doesn't matter whether the list is empty or non-existent, and returning an empty list surely makes it easier for the caller to (non-)iterate over it. But this has nothing to do with the original idea of this ticket: Iff (!) the caller passes default=None to getX(), return None when no value for this option is present.

[osimons]

Nope, I still don't really get the reason for why those defaults needs to be different - other than to match the ConfigParser implementation that we don't match anyway. I prefer our config abstraction to be as simple, consistent and idiot-proof to use as possible for plugin developers.

The only very useful use-case I can see is of course where we want to know if an option is actually defined in files or exists in option registry. For that I would then prefer that we implemented config.has_option() instead…

Anyway, I have no particular reason or interest in making more out of this. I'm happy with any solution providing consistent answers.

[osimons]

Updated patch - see ticket comment for changes.

[osimons]

Added a new patch​ that:

• Sticks to always returning 'type-defaults' when no value is found.
• Updates default to indicate type.
• Introduces a config.has_option() that checks project file, parent file and registry to see if a config exists. Perhaps rename to exists() as it checks more than corresponding sections() and options() sibling methods? Less confusing for anyone with ConfigParser in mind as in options() will not always return the same as has_option(). I think I like exists() better.
• Changes a unit test for getint() that now returns 0 (zero) instead of None.
• Adds a unit test for has_option().
• Fixes a docstring for a skip_empty that is now keep_empty.

All tests pass, at least. Have not figured out how to query the Option.registry for values - is that possible? I don't see any other tests doing it.

If we're heading this way I'll review all docs again, and also add a unit test for the strange-behaving list default in previous comment + some other defaults from my tables if not already covered.

[thomas.moschny@…]

Don't use mutable default arguments (default=[]), see ​10 Python pitfalls, section 5.

[osimons]

Updated patch, standardising on string as default. Also more docs + more tests.

[osimons]

Replying to thomas.moschny@gmx.de:

Don't use mutable default arguments (default=[]), see ​10 Python pitfalls, section 5.

Good point. For our quite simple useage, I am sure we could have worked around that easily. However, if picking something else, string is the best default for all methods. A config value, be it in a file or in option registry, has string as its universal representation. For our 4 main get*() methods, there are two important conveniences:

1. You get the signified type back as return value.
2. You can pass in a default value that is of the same type as expected return value.

See t4977-r6271-config_defaults-a.diff​ for implementation. It is a cumulated patch, including other relevant changes from 14 like config.has_option().

[Christian Boos]

I'm +1 on the latest patch, I think it's fine for 0.11.

[osimons]

Latest patch committed as [6280]. Closing.