Skip to content
This repository has been archived by the owner on Nov 30, 2024. It is now read-only.

Add support for optimized requires. #45

Merged
merged 3 commits into from
Feb 24, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions lib/rspec/support.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,33 @@
require "rspec/support/version"

module RSpec
module Support
# @api private
#
# Defines a helper method that is optimized to require files from the
# named lib. The passed block MUST be `{ |f| require_relative f }`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear explanation, but that is a weird declaration requirement 😀

Now I know another good use case for the binding_of_caller gem… (but since we control all the code here, there's really no need).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about passing in __FILE__ and building a require to the absolute path, which should also shortcut the lookup?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered about passing in FILE and building a require to the absolute path, which should also shortcut the lookup?

I think that would probably work, but I dislike it for a few versions:

  • Years of doing ruby has taught me that absolute requires are rarely what you want.
  • Mixing absolute and relative requires can often lead to pain, where it allows a file to be loaded twice if you reference it in two different ways
  • require_relative is the 1.9+ way of doing this; why re-invent it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less code to repeat ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but Ok I'm convinced

# because for `require_relative` to work properly from within the named
# lib the line of code must be IN that lib.
#
# `require_relative` is preferred when available because it is always O(1),
# regardless of the number of dirs in $LOAD_PATH. `require`, on the other
# hand, does a linear O(N) search over the dirs in the $LOAD_PATH until
# it can resolve the file relative to one of the dirs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A++ would read again

def self.define_optimized_require_for_rspec(lib, &require_relative)
name = "require_rspec_#{lib}"

if Kernel.respond_to?(:require_relative)
(class << self; self; end).__send__(:define_method, name) do |f|
require_relative.call("#{lib}/#{f}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadowing require_relative here was confusing to me, took me a minute to figure out what was going on. block, while less descriptive, would be more obvious. require_relative_block is another option, though too long to be my first choice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see your point, but I think I still like require_relative as the variable name, given that that's what the block needs to be. At least any possible confusion is entirely localized to this method.

end
else
(class << self; self; end).__send__(:define_method, name) do |f|
require "rspec/#{lib}/#{f}"
end
end
end

define_optimized_require_for_rspec(:support) { |f| require_relative(f) }
require_rspec_support "version"

# @api private
KERNEL_METHOD_METHOD = ::Kernel.instance_method(:method)

Expand Down
7 changes: 4 additions & 3 deletions lib/rspec/support/spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'rspec/support/spec/deprecation_helpers'
require 'rspec/support/spec/with_isolated_stderr'
require 'rspec/support/spec/stderr_splitter'
require 'rspec/support'
RSpec::Support.require_rspec_support "spec/deprecation_helpers"
RSpec::Support.require_rspec_support "spec/with_isolated_stderr"
RSpec::Support.require_rspec_support "spec/stderr_splitter"

warning_preventer = $stderr = RSpec::Support::StdErrSplitter.new($stderr)

Expand Down
4 changes: 1 addition & 3 deletions spec/rspec/support/spec/stderr_splitter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@
end

it 'conforms to the stderr interface' do
stderr.methods.each do |method_name|
expect(splitter).to respond_to method_name
end
expect(splitter).to respond_to(*stderr.methods)
end

it 'behaves like stderr' do
Expand Down