In a comment to Hack-svn && Ship-svn, Yossef Mendelssohn of OG Consulting, suggested that my chosen workflow may not be ideal. He probably knows much better than I. Here's the (relevent portion of the) comment:

Also, about your git-svn scripts. Kevin (the third OG) was working on the same thing, even going so far as to make 'hack' and 'ship' DTRT based on whether you're in a git-svn repo or a regular git repo. In my experience, however, there's more difference between vanilla git and git-svn than just whether you git pull or git svn rebase. I've had enough troubles that I no longer dcommit from master. I always keep master as the state of the svn repo and dcommit from branches. Much fewer headaches that way.

(Note that the three OGs are Yossef Mendelssohn, Rick Bradley, and Kevin Barnes.)

I had previously seen a similar idea suggested by a commentor to Andy Delcambre's 'Git SVN Workflow' by the name of Chris McGrath:

That workflow is very similar to how I use git-svn. Something I don’t do that I’ve seen in nearly every article like this is the merge back to master. I just git-svn rebase my topic branch to svn HEAD, fix any conflicts there and git-svn dcommit that, possibly doing a rebase -i beforehand to clean up the commit(s). Then I switch to master, rebase again to get the committed changes and delete the topic branch. My reasons for this aren’t really biggies imho, more that this is the way I’ve always done it. They are:

1. Master will always contain only changes that are in svn, so easy to make a quick bugfix branch without having to work out which rev to branch from. 2. The git-svn man page doesn’t recommend doing git merges, but using dcommit or format patch to move changes between git branches. 3. It’s one less place to get conflicts, as they will only happen on the topic branch rebase. ...

Fair enough.

So, that means what?

Well, following the basics of Yossef's suggestion would make ship-svn really, really boring. (That's not to say it is incorrect, or ineffective.)

#!/bin/sh -x
CURRENT=`git name-rev HEAD --name-only`
git svn dcommit

Right? Should we also checkout master and 'git svn rebase' again? That step would get done the next time you run 'hack-svn' anyway. Hmm. Maybe there is another, better workflow of which I am unaware?

Mr. McGrath is suggesting something like:

#!/bin/sh -x
CURRENT=`git name-rev HEAD --name-only`
git svn rebase
git svn dcommit
git checkout master
git svn rebase
git branch -D ${CURRENT}

I don't really like that. However, I do have a nasty habit of working out of the same 'topic' branch constantly, rather than naming a new one with each new feature. Sometimes I do feel the need to have a few different branches active, if I am unsure of the best path, and I intend to discover it through experimentation. Most of the time, I just work out of 'devel'. That approach makes the adaptation of the original Hack && Ship method pleasing, to me.

Again, that probably means I'm wrong. I will likely adopt my interpretation of Yossef's method soon, while sticking with the original for now. I'd like to think about it for a bit. I would also enjoy hearing what others think.

Once in a while, it's nice to actually go back and look at old code. Not because that old code makes you feel good about yourself at all -- quite the opposite -- but because you feel good about yourself after you clean it up a little bit.

Ola Bini's Evil Hook Methods? got me thinking about how I extend classes in a dusty Rails application. It reminded me of Jay Fields' Ruby: Underuse of Modules. Of course, Jay also weighed in on Ola's concerns. All of these articles have led me to take a look at some simple extensions I had made to Time for this particular Dusty Rails App (DRA, from now on). First, this is what I had before:

Time.class_eval do 
  def self.next_sunday
    t = Time.local(Time.now.year, Time.now.month, Time.now.day, 0) # Today at midnight
    t += 60*60*24 until t.wday == 0
    t # Should now be next Sunday at midnight
  end

  def self.nth_wday(n, wday, month, year)
    if (!n.between? 1,5) ||
       (!wday.between? 0,6) ||
       (!month.between? 1,12)
      raise ArgumentError
    end
    t = Time.local year, month, 1
    first = t.wday
    if first == wday
      fwd = 1
    elsif first < wday
      fwd = wday - first + 1
    elsif first > wday
      fwd = (wday+7) - first + 1
    end
    target = fwd + (n-1)*7
    begin
      t2 = Time.local year, month, target
    rescue ArgumentError
      return nil
    end
    if t2.mday == target
      t2
    else
      nil
    end
  end

  def after_hours?
    result = true
    # M-F, 8am to 6pm are business hours
    if (1..5).include?(self.wday) && (8..18).include?(self.hour) 
      result = false
    end
    # unless it's a holiday!
    if self.holiday?
      result = true
    end
    result
  end

  def holiday?
    unless new_years_day? || 
       mlk_jr_day? ||
       memorial_day? ||
       independence_day? ||
       labor_day? ||
       thanksgiving? ||
       christmas_day?
      return false
    end
    true
  end

  def new_years_day?
    return false unless self.yday == 1
    true
  end

  def mlk_jr_day?
    return false unless self.yday == Time.nth_wday(3,1,1,self.year)
    true
  end

  def memorial_day?
    last_day_of_month = Time.local(self.year, 5, 31)
    d = last_day_of_month.mday - last_day_of_month.wday + 1
    return false unless Time.now.yday == Time.local(self.year, 5, d)
    true
  end

  def indepence_day?
    return false unless self.month == 7 && self.mday == 4
    true
  end

  def labor_day?
    return false unless self.yday == Time.nth_wday(1,1,9,self.year).yday
    true
  end

  def thanksgiving?
    return false unless self.yday == self.class.nth_wday(4,4,11,self.year).yday 
    true
  end

  def christmas_day?
    return false unless self.month == 12 && self.mday == 25
    true
  end
end

I see only trees. Where is the forest?

What we're worried about is how Time was extended. It was a simple class_eval -- just open the class up and insert these methods. The file is simply required in config/environment.rb. In this case, I am adding functionality that didn't exist, so it could be left alone. It's not that big of a deal. As long as you're looking at the file that contains this code, understanding what is happening is pretty simple.

I would like to move these methods to a module. Time could then include/extend them. I do like to have modules in the lib/ directory while using initializers to pull them into DRA. That method of organization seems much cleaner to me. Plus, I can't very well end this article by telling you that I'm just going to leave DRA alone. Not now that we've come this far.

Modularize It Already!

module NframeTimeExtensions
  module ClassMethods
    def next_sunday
      t = Time.local(Time.now.year, Time.now.month, Time.now.day, 0) # Today at midnight
      t += 60*60*24 until t.wday == 0
      t # Should now be next Sunday at midnight
    end

    def nth_wday(n, wday, month, year)
      if (!n.between? 1,5) ||
         (!wday.between? 0,6) ||
         (!month.between? 1,12)
        raise ArgumentError
      end
      t = Time.local year, month, 1
      first = t.wday
      if first == wday
        fwd = 1
      elsif first < wday
        fwd = wday - first + 1
      elsif first > wday
        fwd = (wday+7) - first + 1
      end
      target = fwd + (n-1)*7
      begin
        t2 = Time.local year, month, target
      rescue ArgumentError
        return nil
      end
      if t2.mday == target
        t2
      else
        nil
      end
    end
  end

  def after_hours?
    result = true
    # M-F, 8am to 6pm are business hours
    if (1..5).include?(self.wday) && (8..18).include?(self.hour) 
      result = false
    end
    # unless it's a holiday!
    if self.holiday?
      result = true
    end
    result
  end

  def holiday?
    unless new_years_day? || 
       mlk_jr_day? ||
       memorial_day? ||
       independence_day? ||
       labor_day? ||
       thanksgiving? ||
       christmas_day?
      return false
    end
    true
  end

  def new_years_day?
    return false unless self.yday == 1
    true
  end

  def mlk_jr_day?
    return false unless self.yday == Time.nth_wday(3,1,1,self.year)
    true
  end

  def memorial_day?
    last_day_of_month = Time.local(self.year, 5, 31)
    d = last_day_of_month.mday - last_day_of_month.wday + 1
    return false unless Time.now.yday == Time.local(self.year, 5, d)
    true
  end

  def indepence_day?
    return false unless self.month == 7 && self.mday == 4
    true
  end

  def labor_day?
    return false unless self.yday == Time.nth_wday(1,1,9,self.year).yday
    true
  end

  def thanksgiving?
    return false unless self.yday == self.class.nth_wday(4,4,11,self.year).yday 
    true
  end

  def christmas_day?
    return false unless self.month == 12 && self.mday == 25
    true
  end
end

Ah, there. The module ClassMethods pattern is used throughout Rails, so you've likely seen it before. Other than that, it's pretty straight-forward. I hope.

Now I need to get it into DRA. So, in RAILSROOT/config/initializers/heyletsincludethosestupidtime_extensions.rb:

require 'nframe_time_extensions'

Time.class_eval do
  include NframeTimeExtensions
  extend NframeTimeExtensions::ClassMethods
end

That smells better to me.

Whoa. Way More Code.

Yeah, this does take more code, and an extra file. I don't mind writing a bit of extra code when I believe that it will make the code clearer. As I said before, this method of organization seems much clearer to me.

It was also a fun little exercise in extending via modules instead of directly adding to classes. That is not a bad thing to practice.

If you haven't looked into Shoulda, maybe you should. It's a plugin (and now gem) that works "on top of" Test::Unit. It truly does make it "easy to write elegant, understandable, and maintainable tests".

The following are a few Shoulda macros that I use for the plugins acts_as_ferret, acts_as_taggable_on_steroids, and acts_as_list. I recommend creating a file 'test/shoulda_macros/plugins.rb', and placing these macros in there. For example:

class Test::Unit::TestCase

# Shoulda macros here

end

Acts As Ferret

  def self.should_act_as_ferret(*fields)
    klass = self.name.gsub(/Test$/, '').constantize

    should "include ActsAsFerret methods" do
      assert klass.extended_by.include?(ActsAsFerret::ClassMethods)
      assert klass.include?(ActsAsFerret::InstanceMethods)
      assert klass.include?(ActsAsFerret::MoreLikeThis::InstanceMethods)
      assert klass.include?(ActsAsFerret::ResultAttributes)
    end

    fields.each do |f|
      should "create an index for field named #{f}" do
        assert klass.aaf_configuration[:fields].include?(f)
      end
    end
  end

Use it like:

should_act_as_ferret :any, :fields, :i_may, :have, :specified

Acts As Taggable On Steroids

  def self.should_act_as_taggable_on_steroids
    klass = self.name.gsub(/Test$/, '').constantize

    should "include ActsAsTaggableOnSteroids methods" do
      assert klass.extended_by.include?(ActiveRecord::Acts::Taggable::ClassMethods)
      assert klass.extended_by.include?(ActiveRecord::Acts::Taggable::SingletonMethods)
      assert klass.include?(ActiveRecord::Acts::Taggable::InstanceMethods)
    end

    should_have_many :taggings, :tags
  end

Acts As List

  def self.should_act_as_list
    klass = self.name.gsub(/Test$/, '').constantize

    context "To support acts_as_list" do
      should_have_db_column('position', :type => :integer)
    end

    should "include ActsAsList methods" do
      assert klass.include?(ActiveRecord::Acts::List::InstanceMethods)
    end

    should_have_instance_methods :acts_as_list_class, :position_column, :scope_condition
  end

Note: These tests are quite basic. Except for checking the fields on acts_as_ferret, there isn't any testing of each plugin's options.

These macros are available at http://github.com/mileszs/shoulda_macros. You can find more macros at the Shoulda wiki on GitHub, http://github.com/thoughtbot/shoulda/wikis/example-macros. If you write some of your own shoulda macros, you should link to them on the wiki as well.