environment : simplified hierarchy according to comments in review

5 files changed, 24 insertions, 44 deletions

diff --git a/lib/spack/spack/environment.py b/lib/spack/spack/environment.py
index b557bf0bbc..3ee917fcec 100644
--- a/lib/spack/spack/environment.py
+++ b/lib/spack/spack/environment.py
@@ -3,73 +3,54 @@ import os.path
 import collections
 
 
-class AttributeHolder(object):
-    """
-    Policy that permits to store any kind of attribute on self. The attributes must be passed as key/value pairs
-    during the initialization of the instance.
-    """
-    def __init__(self, **kwargs):
-        for key, value in kwargs.items():
-            setattr(self, key, value)
+class NameModifier(object):
+    def __init__(self, name, **kwargs):
+        self.name = name
+        self.args = {'name': name}
+        self.args.update(kwargs)
 
 
-class SetEnv(AttributeHolder):
+class NameValueModifier(object):
     def __init__(self, name, value, **kwargs):
-        super(SetEnv, self).__init__(**kwargs)
         self.name = name
         self.value = value
+        self.args = {'name': name, 'value': value}
+        self.args.update(kwargs)
 
+
+class SetEnv(NameValueModifier):
     def execute(self):
         os.environ[self.name] = str(self.value)
 
 
-class UnsetEnv(AttributeHolder):
-    def __init__(self, name, **kwargs):
-        super(UnsetEnv, self).__init__(**kwargs)
-        self.name = name
-
+class UnsetEnv(NameModifier):
     def execute(self):
         os.environ.pop(self.name, None)  # Avoid throwing if the variable was not set
 
 
-class AppendPath(AttributeHolder):
-    def __init__(self, name, path, **kwargs):
-        super(AppendPath, self).__init__(**kwargs)
-        self.name = name
-        self.path = path
-
+class AppendPath(NameValueModifier):
     def execute(self):
         environment_value = os.environ.get(self.name, '')
         directories = environment_value.split(':') if environment_value else []
         # TODO : Check if this is a valid directory name
-        directories.append(os.path.normpath(self.path))
+        directories.append(os.path.normpath(self.value))
         os.environ[self.name] = ':'.join(directories)
 
 
-class PrependPath(AttributeHolder):
-    def __init__(self, name, path, **kwargs):
-        super(PrependPath, self).__init__(**kwargs)
-        self.name = name
-        self.path = path
-
+class PrependPath(NameValueModifier):
     def execute(self):
         environment_value = os.environ.get(self.name, '')
         directories = environment_value.split(':') if environment_value else []
         # TODO : Check if this is a valid directory name
-        directories = [os.path.normpath(self.path)] + directories
+        directories = [os.path.normpath(self.value)] + directories
         os.environ[self.name] = ':'.join(directories)
 
 
-class RemovePath(AttributeHolder):
-    def __init__(self, name, path, **kwargs):
-        super(RemovePath, self).__init__(**kwargs)
-        self.name = name
-        self.path = path
-
+class RemovePath(NameValueModifier):
     def execute(self):
         environment_value = os.environ.get(self.name, '')
         directories = environment_value.split(':') if environment_value else []
-        directories = [os.path.normpath(x) for x in directories if x != os.path.normpath(self.path)]
+        directories = [os.path.normpath(x) for x in directories if x != os.path.normpath(self.value)]
         os.environ[self.name] = ':'.join(directories)
 
 
diff --git a/lib/spack/spack/modules.py b/lib/spack/spack/modules.py
index 09895d8c44..0f826c7363 100644
--- a/lib/spack/spack/modules.py
+++ b/lib/spack/spack/modules.py
@@ -178,7 +178,7 @@ class EnvModule(object):
     def process_environment_command(self, env):
         for command in env:
             # FIXME : how should we handle errors here?
-            yield self.formats[type(command)].format(command)
+            yield self.formats[type(command)].format(**command.args)
 
     @property
     def file_name(self):
@@ -203,8 +203,8 @@ class Dotkit(EnvModule):
     path = join_path(spack.share_path, "dotkit")
 
     formats = {
-        PrependPath: 'dk_alter {0.name} {0.path}\n',
-        SetEnv: 'dk_setenv {0.name} {0.value}\n'
+        PrependPath: 'dk_alter {name} {value}\n',
+        SetEnv: 'dk_setenv {name} {value}\n'
     }
 
     @property
@@ -238,8 +238,8 @@ class TclModule(EnvModule):
     path = join_path(spack.share_path, "modules")
 
     formats = {
-        PrependPath: 'prepend-path {0.name} \"{0.path}\"\n',
-        SetEnv: 'setenv {0.name} \"{0.value}\"\n'
+        PrependPath: 'prepend-path {name} \"{value}\"\n',
+        SetEnv: 'setenv {name} \"{value}\"\n'
     }
 
     @property
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 8e56dd1f97..680b26d69a 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -979,6 +979,7 @@ class Package(object):
 
     def setup_environment(self, env):
         """
+
         Called before the install() method of dependents.
 
         Return the list of environment modifications needed by dependents (or extensions). Default implementation does
diff --git a/lib/spack/spack/test/environment.py b/lib/spack/spack/test/environment.py
index d0b093b054..3e03760c01 100644
--- a/lib/spack/spack/test/environment.py
+++ b/lib/spack/spack/test/environment.py
@@ -53,7 +53,7 @@ class EnvironmentTest(unittest.TestCase):
         env = EnvironmentModifications()
         env.set_env('A', 'dummy value', who='Pkg1')
         for x in env:
-            assert hasattr(x, 'who')
+            assert 'who' in x.args
         env.apply_modifications()
         self.assertEqual('dummy value', os.environ['A'])
 
diff --git a/lib/spack/spack/util/environment.py b/lib/spack/spack/util/environment.py
index 1485992b0f..55e653fd2f 100644
--- a/lib/spack/spack/util/environment.py
+++ b/lib/spack/spack/util/environment.py
@@ -40,13 +40,11 @@ def env_flag(name):
     return False
 
 
-# FIXME : remove this function ?
 def path_set(var_name, directories):
     path_str = ":".join(str(dir) for dir in directories)
     os.environ[var_name] = path_str
 
 
-# FIXME : remove this function ?
 def path_put_first(var_name, directories):
     """Puts the provided directories first in the path, adding them
        if they're not already there.