Extract ExternalCommand and CleanedOutput classes #63

chrismytton · 2017-02-07T18:58:52Z

This adds two new classes, ExternalCommand, which runs external commands and captures their output, and CleanedOutput, which cleans up the output from external commands ready for inclusion in a pull request.

Fixes #43
Fixes #45
Part of #62

tmtmtmtm · 2017-02-07T21:02:59Z

README.md

+
+The main task this app performs is running the EveryPolitician build process as an external command and then creating a pull request with the output from the command. As such, one of the main classes in the system is the `ExternalCommand` class. This encapsulates running an external command, checking for errors and returning the output.
+
+To use this class create an instance of `ExternalCommand` and pass `command` and `end` keyword arguments to the constructor. The `command` should be a string representing the external command to execute, and `env` is an optional hash containing environment variables that should be set, or if they are `nil`, unset.


typo: end → env

This outlines the expected usage for the ExternalCommand class.

This class encapsulates running an external command and gathering up the output.

This class is responsible for taking the output from an external command and cleaning it up ready to be included in a GitHub comment.

This simplifies the `RebuilderJob#perform` method quite a bit, and means that we're using tested classes, rather than just having a big ball of code.

Rather than needing to override ENV we instead allow passing in a Morph API key, and then defaulting that to an environment variable.

Previously this was accidentally replacing every character with REDACTED when the API key was "".

chrismytton · 2017-02-08T12:30:49Z

Oops, requested a review before I'd updated the title and body. Fixed now.

tmtmtmtm

Yay! This is definitely a lot better. A few interface/implementation notes in-place

tmtmtmtm · 2017-02-08T12:40:56Z

app.rb

@@ -49,36 +50,28 @@ def perform(country_slug, legislature_slug, source = nil)
    branch_parts = [country_slug, legislature_slug, Time.now.to_i]
    branch = branch_parts.join('-').parameterize
    message = "#{country.name}: Refresh from upstream changes"
-    output = ''
-    child_status = nil
+    command = nil


This raises a few warning signs to me. In general, assigning a placeholder that you're going to re-assign later is quite suspicious, and here's not really obvious why it's needed.

I think it's also a little badly named, as it's named for what it is (which is obvious as it's going be an ExternalCommand object), rather than what it's doing (i.e. it's like naming your variables integer or string), and leads to some confusion when it's later used again at line 68, even though there's been several other commands executed in the meantime.

Perhaps it would be clearer and simpler to hoist the declaration of this command (if not others) out of within the with_git_repo blocks, and then only trigger the .run inside?

tmtmtmtm · 2017-02-08T12:44:35Z

lib/cleaned_output.rb

+require 'colorize'
+
+class CleanedOutput
+  def initialize(output:, morph_api_key: ENV['MORPH_API_KEY'])


it feels a little odd to me to have this class so tightly coupled to Morph-specific concepts like this, or for a class like this to be peering into ENV. This seems like it would be a lot more extensible if the argument here was something like redact: *strings, for a list of things you want redacted in the output.

tmtmtmtm · 2017-02-08T12:49:18Z

lib/cleaned_output.rb

+
+  attr_reader :output, :morph_api_key
+
+  def cleaned_output


this is overloading what 'cleaned' means in a slightly odd way, where cleaned_output here is something different to the CleanedOutput of the class. This is really just the original output with redactions applied.

tmtmtmtm · 2017-02-08T12:50:18Z

lib/cleaned_output.rb

+  end
+
+  def to_s
+    (cleaned_output[-64_000..-1] || cleaned_output).uncolorize


I suspect this would be a little clearer if the truncation code were pulled out into its own method.

The magic number here should probably be hoisted out. (We'd probably want to take the size as an optional constructor argument too, but I'm OK with leaving that part to a later enhancement if it's not as trivial as it seems it should be)

chrismytton · 2017-02-08T16:12:14Z

This pull request has got a bit large for my liking. I've broken off the CleanedOutput change into #64 so that can be reviewed separately. That pull request addresses the feedback on the CleanedOutput class from above.

chrismytton · 2017-02-08T17:33:19Z

#64 has now been merge into master, that contains the output cleaner part of this PR. So the next step is to pull the external command bits into another branch off master.

chrismytton · 2017-02-10T15:08:31Z

CleanedOutput work was done in #64 and #65. The ExternalCommand work is being done in #72. So this PR is now redundant.

chrismytton added the 3 - WIP label Feb 7, 2017

tmtmtmtm reviewed Feb 7, 2017

View reviewed changes

chrismytton force-pushed the 45-extract-class-for-running-external-commands branch from 72ef3fe to f6ba631 Compare February 8, 2017 09:29

chrismytton added 3 commits February 8, 2017 11:57

Add some README details about ExternalCommand class

f969f61

This outlines the expected usage for the ExternalCommand class.

Add ExternalCommand class

fcf7527

This class encapsulates running an external command and gathering up the output.

Add CleanedOutput class

c98c8ff

This class is responsible for taking the output from an external command and cleaning it up ready to be included in a GitHub comment.

chrismytton force-pushed the 45-extract-class-for-running-external-commands branch from 3e309c4 to c28d349 Compare February 8, 2017 10:57

Switch app.rb to use ExternalCommand and CleanedOutput classes

8e7e5cf

This simplifies the `RebuilderJob#perform` method quite a bit, and means that we're using tested classes, rather than just having a big ball of code.

chrismytton force-pushed the 45-extract-class-for-running-external-commands branch from c28d349 to 8e7e5cf Compare February 8, 2017 11:50

chrismytton added 3 commits February 8, 2017 12:54

Don't mess with ENV in the tests

91059b0

Rather than needing to override ENV we instead allow passing in a Morph API key, and then defaulting that to an environment variable.

Make sure output is correct when no Morph API key provided

fbb71d1

Previously this was accidentally replacing every character with REDACTED when the API key was "".

Add details to README about CleanedOutput class

e84cc66

chrismytton requested a review from tmtmtmtm February 8, 2017 12:29

chrismytton assigned tmtmtmtm Feb 8, 2017

chrismytton changed the title ~~[WIP] Extract ExternalCommand class~~ Extract ExternalCommand and CleanedOutput classes Feb 8, 2017

tmtmtmtm suggested changes Feb 8, 2017

View reviewed changes

tmtmtmtm assigned chrismytton and unassigned tmtmtmtm Feb 8, 2017

chrismytton mentioned this pull request Feb 8, 2017

Extract class for cleaning output #64

Merged

This was referenced Feb 8, 2017

Refactor RebuilderJob#perform method #62

Open

[WIP] Prototype of moving build logic to shell script #66

Closed

chrismytton closed this Feb 10, 2017

chrismytton removed the 3 - WIP label Feb 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract ExternalCommand and CleanedOutput classes #63

Extract ExternalCommand and CleanedOutput classes #63

chrismytton commented Feb 7, 2017 •

edited

Loading

tmtmtmtm Feb 7, 2017

chrismytton commented Feb 8, 2017

tmtmtmtm left a comment

tmtmtmtm Feb 8, 2017

tmtmtmtm Feb 8, 2017

tmtmtmtm Feb 8, 2017

tmtmtmtm Feb 8, 2017

tmtmtmtm Feb 8, 2017

chrismytton commented Feb 8, 2017

chrismytton commented Feb 8, 2017

chrismytton commented Feb 10, 2017


		The main task this app performs is running the EveryPolitician build process as an external command and then creating a pull request with the output from the command. As such, one of the main classes in the system is the `ExternalCommand` class. This encapsulates running an external command, checking for errors and returning the output.

		To use this class create an instance of `ExternalCommand` and pass `command` and `end` keyword arguments to the constructor. The `command` should be a string representing the external command to execute, and `env` is an optional hash containing environment variables that should be set, or if they are `nil`, unset.

Extract ExternalCommand and CleanedOutput classes #63

Extract ExternalCommand and CleanedOutput classes #63

Conversation

chrismytton commented Feb 7, 2017 • edited Loading

tmtmtmtm Feb 7, 2017

Choose a reason for hiding this comment

chrismytton commented Feb 8, 2017

tmtmtmtm left a comment

Choose a reason for hiding this comment

tmtmtmtm Feb 8, 2017

Choose a reason for hiding this comment

tmtmtmtm Feb 8, 2017

Choose a reason for hiding this comment

tmtmtmtm Feb 8, 2017

Choose a reason for hiding this comment

tmtmtmtm Feb 8, 2017

Choose a reason for hiding this comment

tmtmtmtm Feb 8, 2017

Choose a reason for hiding this comment

chrismytton commented Feb 8, 2017

chrismytton commented Feb 8, 2017

chrismytton commented Feb 10, 2017

chrismytton commented Feb 7, 2017 •

edited

Loading