Use original return values for rake methods

[r6e]

Problem

The Rake instrumentation causes the results of #invoke and #execute to be swallowed. This means, when calling one Rake task from another Rake task, the former will now always return nil. For example, given the following rake tasks:

namespace :some do
  task :task do
    raise 'I am sad.' unless Rake::Task['some:other_task'].execute == 'I am happy!'
    puts 'Yaaay!'
  end

  task :other_task do
    'I am happy!'
  end
end

Running rake some:task will fail with the "I am sad." message.

Impact

There are some instances, where it is helpful to run certain Rake tasks within others. For example, running ActiveRecord's built-in db tasks as part of a larger purpose-built task that sets up an environment.

In general, this does not appear to be a common use-case. However, in cases where it's needed, the only alternative would be to use exceptions for control-flow, which is an anti-pattern that should be avoided.

Solution

Simply assign the results of super in these two methods to a local variable, and return that at the end of the wrapper's execution. This causes rake some:task to print "Yaaay!" and not error, for the above example.

[delner]

Thanks for the heads up @redapted. I'm honestly surprised this is happening. Tracer#trace is supposed to return the return value from the block provided to it. See https://github.com/DataDog/dd-trace-rb/blob/master/lib/ddtrace/tracer.rb#L323 for implementation. Not sure why this is.

EDIT: Oh right, we're calling annotate_invoke! right after, that's why.

A possibly shorter way of doing this would be super.tap { annotate_invoke!(span, args) } instead of creating the local variable.

I would also suggest adding in an RSpec test that verifies we don't regress on this in the future. Otherwise this seems like a good idea to me.

[r6e]

@delner

A possibly shorter way of doing this would be super.tap { annotate_invoke!(span, args) } instead of creating the local variable.

Good call! I always forget about #tap, despite how handy it is.

I would also suggest adding in an RSpec test that verifies we don't regress on this in the future.

Added. Only added an example for #invoke, to keep things DRY, since #invoke returns the return value of #execute anyway. Not quite sure if this example matches the project's idioms (a cursory look seems to suggest so), so please let me know if it should be cleaned up in any way.

The builds for Ruby < 2.3 now seem to be failing on the "Install Appraisal Gems" step, but they were passing with the previous commit.

It looks like the bundle checksums have changed, so it could be that this commit got on the wrong side of a cache expiration or Docker image update? The latest ruby >= 2.1 builds are trying to install nokogiri 1.10.3, has a minimum ruby version of 2.3.0, whereas the second-most-recent build was trying to install nokogiri 1.9.1, which has a minimum ruby version of 2.1.0.

The 2.0 build is failing on hitimes (1.3.1 vs 1.3.0), and 1.9 on dogstatsd-ruby (4.2.0 vs. 3.3.0) for what appear to be similar reasons. Anything I should do to remedy this, or is it a CI issue?

[delner]

@redapted Pretty sure this is flaky CircleCI behavior; whenever I see this, I just re-run the builds and most of the time it resolves itself. Haven't figured out why CircleCI has a habit of trying to install the wrong gems, but as you pointed out, it seems correlated with Ruby version.

Overall the changes look good, and I'm quite happy with this! I think I'll merge this for the next release (probably 0.23.0.)

Thanks for the contribution! 🎉