Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

code review, also, why no tests?

  • Loading branch information...
commit 4564de9236ae020cc3c008023c5086b5af5e4cdc 1 parent 7639611
Bob Briski authored
View
3  app/models/myreplicator/export.rb
@@ -123,6 +123,9 @@ def self.available_dbs
# Handles connecting to multiple databases
##
+ # BOB : Not sure what I think of inner classes
+ # Is this class used anywhere else in the app?
+ # If so, I'd namespace it in it own file (e.g. Export::SourceDb)
class SourceDb < ActiveRecord::Base
def self.connect db
View
3  app/models/myreplicator/log.rb
@@ -69,6 +69,9 @@ def running?
if logs.count > 0
logs.each do |log|
+ # BOB : All you're doing is checking if the pid file exists
+ # If there is a machine failure the file will exist but the process will not
+ # Shouldn't you check if the pid is active?
if File.exists? "/proc/#{log.pid.to_s}"
puts "still running #{filepath}"
return true
View
6 lib/exporter/export_metadata.rb
@@ -32,6 +32,7 @@ def initialize *args
end
end
+ # BOB : This only handles gzipped files, is that what you want?
def filename
name = filepath.split("/").last
name = zipped ? "#{name}.gz" : name
@@ -160,6 +161,8 @@ def export_path
return path
end
+ # BOB : I don't understand this, why are you using the command line to echo to a file
+ # rather than just store the json directly to a file?
def store!
cmd = "echo \"#{self.to_json.gsub("\"","\\\\\"")}\" > #{@filepath}.json"
puts cmd
@@ -175,6 +178,9 @@ def load metadata_path
def set_attributes options
options.symbolize_keys!
+
+ # BOB : Check out instance_variable_set
+ # I think it will help you here
@export_time = options[:export_time] if options[:export_time]
@table = options[:table] if options[:table]
@database = options[:database] if options[:database]
View
4 lib/exporter/sql_commands.rb
@@ -144,7 +144,9 @@ def self.mysql_export_outfile *args
flags += " --#{flag} "
end
end
-
+
+ # BOB : You always build the host,password,username part of the command
+ # Seems like this should be in a function somewhere
cmd = Myreplicator.mysql
cmd += "#{flags} -u#{db_configs(db)["username"]} -p#{db_configs(db)["password"]} "
cmd += "-h#{db_host} " if db_configs(db)["host"].blank?
View
5 lib/myreplicator.rb
@@ -47,6 +47,11 @@ class Engine < Rails::Engine
end
+ # BOB : Usually you'd make a Myreplicator::Error inherit from StandardError,
+ # then make all of the other exceptions inherit from Myreplicator::Error
+ # With the way you have it set up, I can't catch all Myreplicator errors without specifying
+ # each individually.
+ # Check out how ActiveRecord sets up its errors for an example
module Exceptions
class MissingArgs < StandardError; end
class ExportError < StandardError; end
Please sign in to comment.
Something went wrong with that request. Please try again.