Refactor BaseEnvironment

readthedocs/doc_builder/environments.py

@@ -73,7 +73,7 @@ class BuildCommand(BuildCommandResultMixin):
 
     def __init__(self, command, cwd=None, shell=False, environment=None,
                  combine_output=True, input_data=None, build_env=None,
-                 bin_path=None, description=None, record_as_success=False):
+                 bin_path=None, description=None):
         self.command = command
         self.shell = shell
         if cwd is None:
@@ -96,7 +96,6 @@ def __init__(self, command, cwd=None, shell=False, environment=None,
         self.description = ''
         if description is not None:
             self.description = description
-        self.record_as_success = record_as_success
         self.exit_code = None
 
     def __str__(self):
@@ -184,21 +183,12 @@ def get_command(self):
 
     def save(self):
         """Save this command and result via the API."""
-        exit_code = self.exit_code
-
-        # Force record this command as success to avoid Build reporting errors
-        # on commands that are just for checking purposes and do not interferes
-        # in the Build
-        if self.record_as_success:
-            log.warning('Recording command exit_code as success')
-            exit_code = 0
-
         data = {
             'build': self.build_env.build.get('id'),
             'command': self.get_command(),
             'description': self.description,
             'output': self.output,
-            'exit_code': exit_code,
+            'exit_code': self.exit_code,
             'start_time': self.start_time,
             'end_time': self.end_time,
         }
@@ -295,47 +285,37 @@ def __init__(self, project, environment=None):
         self.environment = environment or {}
         self.commands = []
 
-    def record_command(self, command):
+    def pre_run_command(self, cls, cmd, warn_only, kwargs):
+        """
+        Method to be called before the command is executed.
+
+        The command that will be executed can be accessed as
+        the last element of ``self.commands``.
+        """
         pass
 
-    def _log_warning(self, msg):
-        log.warning(LOG_TEMPLATE.format(
-            project=self.project.slug,
-            version='latest',
-            msg=msg,
-        ))
+    def post_run_command(self):
+        """
+        Method to be called after the command is executed.
+
+        The command that was executed can be accessed as
+        the last element of ``self.commands``.
+        """
+        pass
 
     def run(self, *cmd, **kwargs):
         """Shortcut to run command from environment."""
         return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs)
 
-    def run_command_class(
-            self, cls, cmd, record=None, warn_only=False,
-            record_as_success=False, **kwargs):
+    def run_command_class(self, cls, cmd, warn_only=False, **kwargs):
         """
         Run command from this environment.
 
         :param cls: command class to instantiate a command
         :param cmd: command (as a list) to execute in this environment
-        :param record: whether or not to record this particular command
-            (``False`` implies ``warn_only=True``)
         :param warn_only: don't raise an exception on command failure
-        :param record_as_success: force command ``exit_code`` to be saved as
-            ``0`` (``True`` implies ``warn_only=True`` and ``record=True``)
         """
-        if record is None:
-            # ``self.record`` only exists when called from ``*BuildEnvironment``
-            record = getattr(self, 'record', False)
-
-        if not record:
-            warn_only = True
-
-        if record_as_success:
-            record = True
-            warn_only = True
-            # ``record_as_success`` is needed to instantiate the BuildCommand
-            kwargs.update({'record_as_success': record_as_success})
-
+        self.pre_run_command(cls, cmd, warn_only, kwargs)
         # Remove PATH from env, and set it to bin_path if it isn't passed in
         env_path = self.environment.pop('BIN_PATH', None)
         if 'bin_path' not in kwargs and env_path:
@@ -348,12 +328,7 @@ def run_command_class(
         build_cmd = cls(cmd, **kwargs)
         self.commands.append(build_cmd)
         build_cmd.run()
-
-        if record:
-            # TODO: I don't like how it's handled this entry point here since
-            # this class should know nothing about a BuildCommand (which are the
-            # only ones that can be saved/recorded)
-            self.record_command(build_cmd)
+        self.post_run_command()
 
         if build_cmd.failed:
             msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command())
@@ -362,19 +337,42 @@ def run_command_class(
                 msg += u':\n{out}'.format(out=build_cmd.output)
 
             if warn_only:
-                self._log_warning(msg)
+                log.warning(LOG_TEMPLATE.format(
+                    project=self.project.slug,
+                    version='latest',
+                    msg=msg,
+                ))
             else:
                 raise BuildEnvironmentWarning(msg)
         return build_cmd
 
 
-class LocalEnvironment(BaseEnvironment):
+class EnvironmentRecordCommandMixin(object):
+
+    # record, warn_only
+    def pre_run_command(self, cls, cmd, warn_only, kwargs):
+        kwargs.update({
+            # TODO: when is this necessary?
+            'build_env': self,
+        })
+        self.record = kwargs.pop('record', True)
+        self.record_as_success = warn_only
+
+    def post_run_command(self):
+        command = self.commands[-1]
+        if self.record_as_success:
+            command.exit_code = 0
+        if self.record:
+            command.save()
+
+
+class LocalEnvironment(EnvironmentRecordCommandMixin, BaseEnvironment):
 
     # TODO: BuildCommand name doesn't make sense here, should be just Command
     command_class = BuildCommand
 
 
-class BuildEnvironment(BaseEnvironment):
+class BuildEnvironment(EnvironmentRecordCommandMixin, BaseEnvironment):
 
     """
     Base build environment.
@@ -452,29 +450,6 @@ def handle_exception(self, exc_type, exc_value, _):
                 self.failure = exc_value
             return True
 
-    def record_command(self, command):
-        command.save()
-
-    def _log_warning(self, msg):
-        # :'(
-        log.warning(LOG_TEMPLATE.format(
-            project=self.project.slug,
-            version=self.version.slug,
-            msg=msg,
-        ))
-
-    def run(self, *cmd, **kwargs):
-        kwargs.update({
-            'build_env': self,
-        })
-        return super(BuildEnvironment, self).run(*cmd, **kwargs)
-
-    def run_command_class(self, *cmd, **kwargs):  # pylint: disable=arguments-differ
-        kwargs.update({
-            'build_env': self,
-        })
-        return super(BuildEnvironment, self).run_command_class(*cmd, **kwargs)
-
     @property
     def successful(self):
         """Is build completed, without top level failures or failing commands."""  # noqa