patternrubyMinor
Structuring code that shells out to an external script
Viewed 0 times
scriptstructuringshellsthatcodeexternalout
Problem
This code that shells out to an external script could use some input in order to read smoother, I think. Praise and critique are welcome.
module CompanyLib
class S3
attr_reader :output
def set_path local, remote
@path = [local, remote]
end
def get_bucket
/(^[^\/]*)/.match(@path[1])[0]
end
def sync
@output = `aws s3 sync #{@path[0]} s3://#{@path[1]} --acl public-read`
raise Exception, "S3 sync command failed: #{output}" unless $?.success?
end
def parse
self.sync
normalize = output.gsub("\n", "\\n")
normalize.scan(/s3:\/\/#{self.get_bucket}(.*?)\\n/).flatten
end
def sync_out
self.parse
end
end
endSolution
Overall looks like a good start, some thoughts I had:
-
If I were implementing this class I probably wouldn't just store the output as a string, I'd parse it in a way that callers would be able to use it effectively. For example, does it output total time, data transferred, files copied, etc.? If you're going to try to use this data, I think this class (or a class used by this class) should have the responsibility to process that data, not the caller.
-
Some prefer the
-
Never raise
-
In one of the codebases I work on were have an entire class dedicated to executing commands locally. One reason we do this is that you can't isolate stdout from stderror using backticks or
I don't know how widely used or important this class is for your company, so feel free to file any of these under "over-engineering" as appropriate :-).
-
If I were implementing this class I probably wouldn't just store the output as a string, I'd parse it in a way that callers would be able to use it effectively. For example, does it output total time, data transferred, files copied, etc.? If you're going to try to use this data, I think this class (or a class used by this class) should have the responsibility to process that data, not the caller.
-
Some prefer the
system() method over in-line backticks, for readability, but that's preference.-
Never raise
Exception, either raise RuntimeError by omitting the first parameter to raise or fail, or create a new exception class (with RuntimeError as a parent) for your purpose. Similarly you should (in almost all cases) never rescue Exception.-
In one of the codebases I work on were have an entire class dedicated to executing commands locally. One reason we do this is that you can't isolate stdout from stderror using backticks or
system(), so we use the popen4 library and generate a Result class that has the exit code, stdout and stderr. The stderr may be useful if you're trying to relay to the caller what failed exactly. The caller may want to retry a timeout, but not an authentication issue, for example.I don't know how widely used or important this class is for your company, so feel free to file any of these under "over-engineering" as appropriate :-).
Context
StackExchange Code Review Q#63077, answer score: 2
Revisions (0)
No revisions yet.