Closed bdbaddog closed 3 months ago
The usual SCons-y mess here. The internal signature of the Variables
function is:
def Variables(files=None, args=ARGUMENTS):
The documented signature is:
Variables([files, [args]])
The Variables
class initializer, meanwhile, is defined this way:
def __init__(self, files=None, args=None, is_global=True):
It I read that right, it means you could only instantiate a Variables
if you directly called to it, passing is_global=False
, you can't do that through the published API (the Variables
function).
So do I read the @gregnoel comment as: get rid of the singleton-forcing stuff, and potentially make it possible for there to be more than one Variables object? If not, then the docs should possibly receive an update to show it's a singleton, the way is mentioned for DefaultEnvironment
.
And, yes, the way this is written is broken, it doesn't create a singleton. I think the author was expecting to be able to morph self
this way but it doesn't do what's expected, so we're not implementing a singleton now.
It looks like Variables.instance is a class variable and thus is a singleton. Am I reading that wrong?
It turns out... yes - my first glance was also to think "oh, it's a singleton". By the time you're in __init__
, a new instance has already been created. You then set self
in __init__
to the one saved, which presumably intends to morph the new instance into the old one, but it's actually just assigning to the local self
and nothing is then done with that. So it's similar to a common pattern, but not doing it right. Unless I'm reading it completely wrong... I can easily make it into one, but I'm not sure there's any benefit to it.
Swung back and checked - it's definitely producing different instances each time you instantiate Variables. It's dead simple to fix - it's a small stanza in __new__
instead of in __init__
, but it doesn't really seem to matter, since we're already not getting a singleton and stuff still seems to be working. This sort of jig would do it:
def __new__(cls):
if not hasattr(cls, '_instance'):
cls._instance = super().__new__(cls)
return cls._instance
The bugparty note seemed to indicate a decision to just forget it: "Remove all uses of self.instance."
Forgot to link this to a PR since what started as a single PR (which did have a link in the commntary) was split into a series - the broken singelton-ish behavior was removed in PR #4533.
This issue was originally created at: 2009-07-04 00:35:03. This issue was reported by:
simpleton
. simpleton said at 2009-07-04 00:35:03