Resuming the discussion of automated code smell reduction refactorings
from Puppet Camp and the following weeks, this is the first in a
series (actually, the second if you count the pilot episode which
showed how superfluous "defines?"s could be removed).
It comprises three parts:
A) This introductory note (which will be omitted in later refactors)
B) The refactor rule, written in a little DSL which does a glorified
series of finds
and replaces on all the rb files in the git repository
(technically not all; the
auto-generated racc grammars, for instance, are left pristine) and then
commits the results
C) An example commit message generated by running the refactor.
Note that what we're looking to approve / disapprove here is not the
detailed series of changes as with a normal patch (which in some
cases, could be many thousands of lines long) but rather
1) The _idea_ of the refactor (do we want to make this change)
2) The _concrete expression_ of the idea (should the specified series of
changes fulfill the goals of the idea, and nothing more).
I'll be posting the refactor tool and more details about it's use and
operation as we go along; for now, a brief rundown of the features
used in this refactor.
It consists of a single ruby statement that gives the name of the
refactor and the actions needed to perform it. When run, this makes
the changes as described above and commits them (assuming that your
working tree is clean, etc.)
In this case the body is simply a series of term replacements each of
which consists of a pattern (a regular expression with some additional
semantics) and a replacement text. The patterns in this case require
a bit more care than in most of the refactors, as many of the
characters have special meaning in regular expressions, and must be
escaped.
Some of the replacements also have "gotchas" associated with them; for
these tests are provided (inline) to fail the commit unless it behaves
as expected.
Finally, the last replacement has an additional condition (the
"provided" clause) which must be satisfied for the replacement to be
made in any particular case.
I think that about covers it, so on with the show.
------------------------------------------------------------------------------------------
refactor "English names for special globals rather than line-noise" do
replace_terms { like '[$][?]'; with '$CHILD_STATUS';
tests('if $? == 0' => 'if $CHILD_STATUS == 0') }
replace_terms { like '[$][$]'; with '$PID'; }
replace_terms { like '[$]&'; with '$MATCH'; }
replace_terms { like '[$]:(?!:)'; with '$LOAD_PATH';
tests('$::var' => :unchanged, '$:.include?' => '$LOAD_PATH.include?') }
replace_terms { like '[$]!'; with '$ERROR_INFO'; }
replace_terms { like '^(.*)[$]"'; with '\1$LOADED_FEATURES'
provided { |prelude| prelude.balanced_quotes? }
tests(
'"$"' => :unchanged, '$"' => '$LOADED_FEATURES',
'if $"' => 'if $LOADED_FEATURES'
)
}
end
commit 2fc096e7dc1a52b74d671686fb40bf46be2a1f12
Author: Markus Roberts <[email protected]>
Date: Tue Jan 12 10:30:01 2010 -0800
Code smell: English names for special globals rather than line-noise
* Replaced 40 occurances of [$][?] with $CHILD_STATUS
3 Examples:
The code:
if $?.exitstatus != 0
becomes:
if $CHILD_STATUS.exitstatus != 0
The code:
print "%s finished with exit code %s\n" % [host, $?.exitstatus]
becomes:
print "%s finished with exit code %s\n" % [host,
$CHILD_STATUS.exitstatus]
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, $CHILD_STATUS.exitstatus]
* Replaced 4 occurances of [$][$] with $PID
3 Examples:
The code:
Process.kill(:HUP, $$)
becomes:
Process.kill(:HUP, $PID)
The code:
Process.expects(:kill).with(:HUP, $$)
becomes:
Process.expects(:kill).with(:HUP, $PID)
The code:
if pid == $$
becomes:
if pid == $PID
* Replaced 7 occurances of [$]& with $MATCH
3 Examples:
The code:
work.slice!(0, $&.length)
becomes:
work.slice!(0, $MATCH.length)
The code:
if $&
becomes:
if $MATCH
The code:
if $&
becomes:
if $MATCH
* Replaced 27 occurances of [$]:(?!:) with $LOAD_PATH
3 Examples:
The code:
sitelibdir = $:.find { |x| x =~ /site_ruby/ }
becomes:
sitelibdir = $LOAD_PATH.find { |x| x =~ /site_ruby/ }
The code:
$:.unshift "lib"
becomes:
$LOAD_PATH.unshift "lib"
The code:
$:.shift
becomes:
$LOAD_PATH.shift
* Replaced 3 occurances of [$]! with $ERROR_INFO
3 Examples:
The code:
$LOG.fatal("Problem reading #{filepath}: #{$!}")
becomes:
$LOG.fatal("Problem reading #{filepath}: #{$ERROR_INFO}")
The code:
$stderr.puts "Couldn't build man pages: " + $!
becomes:
$stderr.puts "Couldn't build man pages: " + $ERROR_INFO
The code:
$stderr.puts $!.message
becomes:
$stderr.puts $ERROR_INFO.message
* Replaced 3 occurances of ^(.*)[$]" with \1$LOADED_FEATURES
3 Examples:
The code:
unless $".index 'racc/parser.rb'
becomes:
unless $LOADED_FEATURES.index 'racc/parser.rb'
The code:
$".push 'racc/parser.rb'
becomes:
$LOADED_FEATURES.push 'racc/parser.rb'
The code:
$".should be_include("tmp/myfile.rb")
becomes:
$LOADED_FEATURES.should be_include("tmp/myfile.rb")
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
