Skip to content

Commit

Permalink
older versions of ruby dont seem to like const_get used in this fashi…
Browse files Browse the repository at this point in the history 
…on, this is arguably better than eval?
  • Loading branch information
@JonRowe
JonRowe committed Oct 4, 2013
1 parent cbe9ec8 commit f8d73a4
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions lib/rspec/core/configuration.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1031,10 +1031,9 @@ def custom_formatter(formatter_ref)
formatter_ref
elsif string_const?(formatter_ref)
begin
::Kernel.const_get(formatter_ref)
formatter_ref.scan(/(?:::|^)(\w+)/).flatten.inject(Object) { |const,string| const.const_get string }

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Oct 4, 2013

Member

Why use scan().flatten instead of split like I did in the rspec-mocks version of this?

This comment has been minimized.

Sign in to view

Copy link
@JonRowe

JonRowe Oct 4, 2013

Author Member

'::Constant'.split('::') => ["","Constant"]

This comment has been minimized.

Sign in to view

Copy link
@JonRowe

JonRowe Oct 4, 2013

Author Member

E.g I preferred scan,flatten to gsub.split

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Oct 4, 2013

Member

There's a bug in your scan regex that's not in the simple split('::'), though:

irb(main):020:0> const
=> "RSpec::Matchers::Custom::Be们"
irb(main):021:0> const.scan(/(?:::|^)(\w+)/)
=> [["RSpec"], ["Matchers"], ["Custom"], ["Be"]]
irb(main):022:0> const.split('::')
=> ["RSpec", "Matchers", "Custom", "Be们"]

Ruby 1.9+ allows non-ascii characters in constant names (but requires that the first character is A-Z). \w+ doesn't match it properly.

In contrast, split().reject "just works":

"::Foo::Bar::Bazz".split('::').reject(&:empty?)
=> ["Foo", "Bar", "Bazz"]

And is a lot simpler to understand than your regex, to boot.

This comment has been minimized.

Sign in to view

Copy link
@JonRowe

JonRowe Oct 4, 2013

Author Member

Not overly precious about it, gsub.split it is.

This comment has been minimized.

Sign in to view

Copy link
@myronmarston

myronmarston Oct 4, 2013

Member

The gsub isn't needed if you'd rather reject(&:empty?), as I showed above. Not sure which I prefer.

This comment has been minimized.

Sign in to view

Copy link
@JonRowe

JonRowe Oct 4, 2013

Author Member

I went with gsub because I'd rather not loop twice.
rescue NameError
require path_for(formatter_ref)
::Kernel.const_get(formatter_ref)
require( path_for(formatter_ref) ) ? retry : raise
end
end
end
Expand Down

1 comment on commit f8d73a4

@myronmarston
Copy link
Member

Choose a reason for hiding this comment

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

I was going to comment on this...ruby 2.0 is the first version that allows const_get to get qualified constants. We should extract our recursive_const_get logic from rspec-mocks into rspec-support and use that from here:

https://github.com/rspec/rspec-mocks/blob/07f96d4d65d283cd1a4aa66ba4527890f33025aa/lib/rspec/mocks/mutate_const.rb#L59-L63

Please sign in to comment.