From 9ee56f2e67be973da49b1d3f21de1bf87de35e6f Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Fri, 9 Jul 2010 18:05:04 -0700 Subject: Code smell: Inconsistent indentation and related formatting issues * Replaced 163 occurances of defined\? +([@a-zA-Z_.0-9?=]+) with defined?(\1) This makes detecting subsequent patterns easier. 3 Examples: The code: if ! defined? @parse_config becomes: if ! defined?(@parse_config) The code: return @option_parser if defined? @option_parser becomes: return @option_parser if defined?(@option_parser) The code: if defined? @local and @local becomes: if defined?(@local) and @local * Eliminate trailing spaces. Replaced 428 occurances of ^(.*?) +$ with \1 1 file was skipped. test/ral/providers/host/parsed.rb because 0 * Replace leading tabs with an appropriate number of spaces. Replaced 306 occurances of ^(\t+)(.*) with Tabs are not consistently expanded in all environments. * Don't arbitrarily wrap on sprintf (%) operator. Replaced 143 occurances of (.*['"] *%) +(.*) with Splitting the line does nothing to aid clarity and hinders further refactorings. 3 Examples: The code: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] becomes: raise Puppet::Error, "Cannot create %s: basedir %s is a file" % [dir, File.join(path)] The code: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] becomes: Puppet.err "Will not start without authorization file %s" % Puppet[:authconfig] The code: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] becomes: $stderr.puts "Could not find host for PID %s with status %s" % [pid, $?.exitstatus] * Don't break short arrays/parameter list in two. Replaced 228 occurances of (.*) +(.*) with 3 Examples: The code: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) becomes: puts @format.wrap(type.provider(prov).doc, :indent => 4, :scrub => true) The code: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) becomes: assert(FileTest.exists?(daily), "Did not make daily graph for %s" % type) The code: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") becomes: assert(prov.target_object(:first).read !~ /^notdisk/, "Did not remove thing from disk") * If arguments must wrap, treat them all equally Replaced 510 occurances of lines ending in things like ...(foo, or ...(bar(1,3), with \1 \2 3 Examples: The code: midscope.to_hash(false), becomes: assert_equal( The code: botscope.to_hash(true), becomes: # bottomscope, then checking that we see the right stuff. The code: :path => link, becomes: * Replaced 4516 occurances of ^( *)(.*) with The present code base is supposed to use four-space indentation. In some places we failed to maintain that standard. These should be fixed regardless of the 2 vs. 4 space question. 15 Examples: The code: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc becomes: def run_comp(cmd) puts cmd results = [] old_sync = $stdout.sync $stdout.sync = true line = [] begin open("| #{cmd}", "r") do |f| until f.eof? do c = f.getc The code: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( becomes: s.gsub!(/.{4}/n, '\\\\u\&') } string.force_encoding(Encoding::UTF_8) string rescue Iconv::Failure => e raise GeneratorError, "Caught #{e.class}: #{e}" end else def utf8_to_pson(string) # :nodoc: string = string.gsub(/["\\\x0-\x1f]/) { MAP[$&] } string.gsub!(/( The code: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } becomes: end } rvalues: rvalue | rvalues comma rvalue { if val[0].instance_of?(AST::ASTArray) result = val[0].push(val[2]) else result = ast AST::ASTArray, :children => [val[0],val[2]] end } The code: #passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } becomes: # passwdproc = proc { @password } keytext = @key.export( OpenSSL::Cipher::DES.new(:EDE3, :CBC), @password ) File.open(@keyfile, "w", 0400) { |f| f << keytext } The code: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") becomes: end def to_manifest "%s { '%s':\n%s\n}" % [self.type.to_s, self.name, @params.collect { |p, v| if v.is_a? Array " #{p} => [\'#{v.join("','")}\']" else " #{p} => \'#{v}\'" end }.join(",\n") The code: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", becomes: via the augeas tool. Requires: - augeas to be installed (http://www.augeas.net) - ruby-augeas bindings Sample usage with a string:: augeas{\"test1\" : context => \"/files/etc/sysconfig/firstboot\", changes => \"set RUN_FIRSTBOOT YES\", The code: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do becomes: names.should_not be_include("root") end describe "when generating a purgeable resource" do it "should be included in the generated resources" do Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource] @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref) end end describe "when the instance's do not have an ensure property" do The code: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') becomes: describe "when the instance's do not have an ensure property" do it "should not be included in the generated resources" do @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo') Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource] @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref) end end describe "when the instance's ensure property does not accept absent" do it "should not be included in the generated resources" do @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar') The code: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) becomes: func = nil assert_nothing_raised do func = Puppet::Parser::AST::Function.new( :name => "template", :ftype => :rvalue, :arguments => AST::ASTArray.new( :children => [stringobj(template)] ) The code: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") becomes: assert( @store.allowed?("hostname.madstop.com", "192.168.1.50"), "hostname not allowed") assert( ! @store.allowed?("name.sub.madstop.com", "192.168.0.50"), "subname name allowed") The code: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } becomes: assert_nothing_raised { server = Puppet::Network::Handler.fileserver.new( :Local => true, :Config => false ) } The code: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid becomes: 'yay', { :failonfail => false, :uid => @user.uid, :gid => @user.gid } ).returns('output') output = Puppet::Util::SUIDManager.run_and_capture 'yay', @user.uid, @user.gid The code: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' becomes: ).times(1) pkg.provider.expects( :aptget ).with( '-y', '-q', 'remove', 'faff' The code: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs becomes: johnny one two billy three four\n" # Just parse and generate, to make sure it's isomorphic. assert_nothing_raised do assert_equal(text, @parser.to_file(@parser.parse(text)), "parsing was not isomorphic") end end def test_valid_attrs The code: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element| becomes: "testing", :onboolean => [true, "An on bool"], :string => ["a string", "A string arg"] ) result = [] should = [] assert_nothing_raised("Add args failed") do @config.addargs(result) end @config.each do |name, element| --- autotest/rspec.rb | 2 +- autotest/watcher.rb | 130 ++++++++++++++++++++++++++-------------------------- 2 files changed, 66 insertions(+), 66 deletions(-) (limited to 'autotest') diff --git a/autotest/rspec.rb b/autotest/rspec.rb index 707429d2e..bf3d2d939 100644 --- a/autotest/rspec.rb +++ b/autotest/rspec.rb @@ -50,7 +50,7 @@ class Autotest::Rspec < Autotest # ~/.autotest to provide a different spec command then the default # paths provided. def spec_command(separator=File::ALT_SEPARATOR) - unless defined? @spec_command then + unless defined?(@spec_command) then @spec_command = spec_commands.find { |cmd| File.exists? cmd } raise RspecCommandError, "No spec command could be found!" unless @spec_command diff --git a/autotest/watcher.rb b/autotest/watcher.rb index 61baf49d0..028d3fe7d 100644 --- a/autotest/watcher.rb +++ b/autotest/watcher.rb @@ -2,75 +2,75 @@ ENV["WATCHR"] = "1" ENV['AUTOTEST'] = 'true' def run_comp(cmd) - puts cmd - results = [] - old_sync = $stdout.sync - $stdout.sync = true - line = [] - begin - open("| #{cmd}", "r") do |f| - until f.eof? do - c = f.getc - putc c - line << c - if c == ?\n then - results << if RUBY_VERSION >= "1.9" then - line.join + puts cmd + results = [] + old_sync = $stdout.sync + $stdout.sync = true + line = [] + begin + open("| #{cmd}", "r") do |f| + until f.eof? do + c = f.getc + putc c + line << c + if c == ?\n then + results << if RUBY_VERSION >= "1.9" then + line.join else - line.pack "c*" - end - line.clear - end + line.pack "c*" + end + line.clear + end + end end - end ensure - $stdout.sync = old_sync + $stdout.sync = old_sync end results.join end def clear - #system("clear") + #system("clear") end def growl(message, status) - # Strip the color codes - message.gsub!(/\[\d+m/, '') - - growlnotify = `which growlnotify`.chomp - return if growlnotify.empty? - title = "Watchr Test Results" - image = status == :pass ? "autotest/images/pass.png" : "autotest/images/fail.png" - options = "-w -n Watchr --image '#{File.expand_path(image)}' -m '#{message}' '#{title}'" - system %(#{growlnotify} #{options} &) + # Strip the color codes + message.gsub!(/\[\d+m/, '') + + growlnotify = `which growlnotify`.chomp + return if growlnotify.empty? + title = "Watchr Test Results" + image = status == :pass ? "autotest/images/pass.png" : "autotest/images/fail.png" + options = "-w -n Watchr --image '#{File.expand_path(image)}' -m '#{message}' '#{title}'" + system %(#{growlnotify} #{options} &) end def file2specs(file) - %w{spec/unit spec/integration}.collect { |d| - file.sub('lib/puppet', d) + %w{spec/unit spec/integration}.collect { |d| + file.sub('lib/puppet', d) }.find_all { |f| - File.exist?(f) + File.exist?(f) } end def file2test(file) - result = file.sub('lib/puppet', 'test') - return nil unless File.exist?(result) - result + result = file.sub('lib/puppet', 'test') + return nil unless File.exist?(result) + result end def run_spec(command) - clear - result = run_comp(command).split("\n").last - status = result.include?('0 failures') ? :pass : :fail - growl result, status + clear + result = run_comp(command).split("\n").last + status = result.include?('0 failures') ? :pass : :fail + growl result, status end def run_test(command) - clear - result = run_comp(command).split("\n").last - status = result.include?('0 failures, 0 errors') ? :pass : :fail - growl result.split("\n").last rescue nil + clear + result = run_comp(command).split("\n").last + status = result.include?('0 failures, 0 errors') ? :pass : :fail + growl result.split("\n").last rescue nil end def run_test_file(file) @@ -85,16 +85,16 @@ def run_spec_files(files) end def run_all_tests - run_test("rake unit") + run_test("rake unit") end def run_all_specs - run_test("rake spec") + run_test("rake spec") end def run_suite - run_all_tests - run_all_specs + run_all_tests + run_all_specs end watch('spec/spec_helper.rb') { run_all_specs } @@ -108,31 +108,31 @@ watch(%r%^lib/puppet/(.*)\.rb$%) { |md| watch(%r!^spec/lib/spec.*!) { |md| run_all_specs } watch(%r!^spec/lib/monkey_patches/.*!) { |md| run_all_specs } watch(%r{test/.+\.rb}) { |md| - if md[0] =~ /\/lib\// - run_all_tests - else - run_test_file(md[0]) - end + if md[0] =~ /\/lib\// + run_all_tests + else + run_test_file(md[0]) + end } # Ctrl-\ Signal.trap 'QUIT' do - puts " --- Running all tests ---\n\n" - run_suite + puts " --- Running all tests ---\n\n" + run_suite end @interrupted = false # Ctrl-C Signal.trap 'INT' do - if @interrupted then - @wants_to_quit = true - abort("\n") - else - puts "Interrupt a second time to quit; wait for rerun of tests" - @interrupted = true - Kernel.sleep 1.5 - # raise Interrupt, nil # let the run loop catch it - run_suite - end + if @interrupted then + @wants_to_quit = true + abort("\n") + else + puts "Interrupt a second time to quit; wait for rerun of tests" + @interrupted = true + Kernel.sleep 1.5 + # raise Interrupt, nil # let the run loop catch it + run_suite + end end -- cgit