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| --- lib/puppet/network/handler/ca.rb | 10 +++--- lib/puppet/network/handler/fileserver.rb | 62 ++++++++++++++++---------------- lib/puppet/network/handler/report.rb | 3 +- 3 files changed, 37 insertions(+), 38 deletions(-) (limited to 'lib/puppet/network/handler') diff --git a/lib/puppet/network/handler/ca.rb b/lib/puppet/network/handler/ca.rb index de7a976c8..20d971007 100644 --- a/lib/puppet/network/handler/ca.rb +++ b/lib/puppet/network/handler/ca.rb @@ -19,7 +19,7 @@ class Puppet::Network::Handler } def autosign - if defined? @autosign + if defined?(@autosign) @autosign else Puppet[:autosign] @@ -35,12 +35,11 @@ class Puppet::Network::Handler # we only otherwise know how to handle files unless autosign =~ /^\// - raise Puppet::Error, "Invalid autosign value %s" % - autosign.inspect + raise Puppet::Error, "Invalid autosign value %s" % autosign.inspect end unless FileTest.exists?(autosign) - unless defined? @@warnedonautosign + unless defined?(@@warnedonautosign) @@warnedonautosign = true Puppet.info "Autosign is enabled but %s is missing" % autosign end @@ -124,8 +123,7 @@ class Puppet::Network::Handler if @ca.getclientcsr(hostname) Puppet.info "Not replacing existing request from %s" % hostname else - Puppet.notice "Host %s has a waiting certificate request" % - hostname + Puppet.notice "Host %s has a waiting certificate request" % hostname @ca.storeclientcsr(csr) end return ["", ""] diff --git a/lib/puppet/network/handler/fileserver.rb b/lib/puppet/network/handler/fileserver.rb index 84af5ac55..a48d05d91 100755 --- a/lib/puppet/network/handler/fileserver.rb +++ b/lib/puppet/network/handler/fileserver.rb @@ -107,8 +107,7 @@ class Puppet::Network::Handler if hash.include?(:Mount) @passedconfig = true unless hash[:Mount].is_a?(Hash) - raise Puppet::DevError, "Invalid mount hash %s" % - hash[:Mount].inspect + raise Puppet::DevError, "Invalid mount hash %s" % hash[:Mount].inspect end hash[:Mount].each { |dir, name| @@ -159,8 +158,7 @@ class Puppet::Network::Handler def mount(path, name) if @mounts.include?(name) if @mounts[name] != path - raise FileServerError, "%s is already mounted at %s" % - [@mounts[name].path, name] + raise FileServerError, "%s is already mounted at %s" % [@mounts[name].path, name] else # it's already mounted; no problem return @@ -219,8 +217,7 @@ class Puppet::Network::Handler clientip = nil end unless mount.allowed?(client, clientip) - mount.warning "%s cannot access %s" % - [client, file] + mount.warning "%s cannot access %s" % [client, file] raise Puppet::AuthorizationError, "Cannot access %s" % mount end end @@ -283,8 +280,7 @@ class Puppet::Network::Handler when /\[([-\w]+)\]/ name = $1 if newmounts.include?(name) - raise FileServerError, "%s is already mounted as %s in %s" % - [newmounts[name], name, @configuration.file] + raise FileServerError, "%s is already mounted as %s in %s" % [newmounts[name], name, @configuration.file] end mount = Mount.new(name) newmounts[name] = mount @@ -299,8 +295,7 @@ class Puppet::Network::Handler begin mount.path = value rescue FileServerError => detail - Puppet.err "Removing mount %s: %s" % - [mount.name, detail] + Puppet.err "Removing mount %s: %s" % [mount.name, detail] newmounts.delete(mount.name) end end @@ -311,7 +306,10 @@ class Puppet::Network::Handler mount.allow(val) rescue AuthStoreError => detail puts detail.backtrace if Puppet[:trace] - raise FileServerError.new(detail.to_s, + + raise FileServerError.new( + detail.to_s, + count, @configuration.file) end } @@ -321,17 +319,18 @@ class Puppet::Network::Handler mount.info "denying %s access" % val mount.deny(val) rescue AuthStoreError => detail - raise FileServerError.new(detail.to_s, + + raise FileServerError.new( + detail.to_s, + count, @configuration.file) end } else - raise FileServerError.new("Invalid argument '%s'" % var, - count, @configuration.file) + raise FileServerError.new("Invalid argument '%s'" % var, count, @configuration.file) end else - raise FileServerError.new("Invalid line '%s'" % line.chomp, - count, @configuration.file) + raise FileServerError.new("Invalid line '%s'" % line.chomp, count, @configuration.file) end count += 1 } @@ -340,8 +339,7 @@ class Puppet::Network::Handler Puppet.err "FileServer error: Cannot read %s; cannot serve" % @configuration #raise Puppet::Error, "Cannot read %s" % @configuration rescue Errno::ENOENT => detail - Puppet.err "FileServer error: '%s' does not exist; cannot serve" % - @configuration + Puppet.err "FileServer error: '%s' does not exist; cannot serve" % @configuration end unless newmounts[MODULES] @@ -368,9 +366,12 @@ class Puppet::Network::Handler # object... mount = PluginMount.new(PLUGINS) # Yes, you're allowed to hate me for this. - mount.instance_variable_set(:@declarations, - newmounts[PLUGINS].instance_variable_get(:@declarations) - ) + + mount.instance_variable_set( + :@declarations, + + newmounts[PLUGINS].instance_variable_get(:@declarations) + ) newmounts[PLUGINS] = mount end @@ -379,8 +380,7 @@ class Puppet::Network::Handler # pointing to the specific problem. newmounts.each { |name, mount| unless mount.valid? - raise FileServerError, "Invalid mount %s" % - name + raise FileServerError, "Invalid mount %s" % name end } @mounts = newmounts @@ -448,8 +448,7 @@ class Puppet::Network::Handler if client map = clientmap(client) else - Puppet.notice "No client; expanding '%s' with local host" % - path + Puppet.notice "No client; expanding '%s' with local host" % path # Else, use the local information map = localmap() end @@ -465,7 +464,7 @@ class Puppet::Network::Handler # Do we have any patterns in our path, yo? def expandable? - if defined? @expandable + if defined?(@expandable) @expandable else false @@ -515,8 +514,11 @@ class Puppet::Network::Handler # the effort. obj[:audit] = CHECKPARAMS else + obj = Puppet::Type.type(:file).new( + :name => file_path(path, client), + :audit => CHECKPARAMS ) @files[file_path(path, client)] = obj @@ -542,11 +544,11 @@ class Puppet::Network::Handler # Cache this manufactured map, since if it's used it's likely # to get used a lot. def localmap - unless defined? @@localmap + unless defined?(@@localmap) @@localmap = { "h" => Facter.value("hostname"), "H" => [Facter.value("hostname"), - Facter.value("domain")].join("."), + Facter.value("domain")].join("."), "d" => Facter.value("domain") } end @@ -710,7 +712,7 @@ class Puppet::Network::Handler end def path_exists?(relpath, client = nil) - !valid_modules(client).find { |mod| mod.plugin(relpath) }.nil? + !valid_modules(client).find { |mod| mod.plugin(relpath) }.nil? end def valid? @@ -749,7 +751,7 @@ class Puppet::Network::Handler end def add_to_filetree(f, filetree) - first, rest = f.split(File::SEPARATOR, 2) + first, rest = f.split(File::SEPARATOR, 2) end end end diff --git a/lib/puppet/network/handler/report.rb b/lib/puppet/network/handler/report.rb index 8cfd8d83d..960b65f73 100755 --- a/lib/puppet/network/handler/report.rb +++ b/lib/puppet/network/handler/report.rb @@ -68,8 +68,7 @@ class Puppet::Network::Handler if Puppet[:trace] puts detail.backtrace end - Puppet.err "Report %s failed: %s" % - [name, detail] + Puppet.err "Report %s failed: %s" % [name, detail] end else Puppet.warning "No report named '%s'" % name -- cgit