Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Some code refactorings #346

Closed
wants to merge 5 commits into from

4 participants

@rosenfeld

While looking at the Rspec code for understanding how it works, I decided to propose some minor refactorings that make the code clearer in my opinion.

Please, take a look

@dchelimsky
Owner

Unfortunately, we can't use tap as long as we're supporting Ruby 1.8.6 :( Wanna redo this without tap?

@rosenfeld

I thought about this, but grep showed me that tap was used in core/subject.rb. Now I've seen it was used in some documentation and not actually in the code... I can use another approach, but I would like to know if you intend to cache @options in parse_options some day (@options ||= ...). If you don't need this, I can rearrange the code easily.

@markiz

What's the motivation for supporting 1.8.6 btw?

@dchelimsky
Owner

@markiz - same motivation as 1.9.1. Some people still use it :)

@myronmarston

One option we can consider is adding a definition of tap for 1.8.6. I'm generally against gems adding new methods to objects they don't own (unless that is the primary purpose of the gem), but I don't mind backporting methods that are now standard in newer versions of ruby. tap is useful and standard and I think the chances are extremely small that conditionally adding it (based on if Object.new.respond_to?(:tap)) would cause problems for end users.

@dchelimsky
Owner

@myronmarston - I'm all for that. We've done the same with instance_exec. Wanna take that on?

@rosenfeld

I don't like this idea. That way, some specs could pass with Rspec but the program could fail under production when on Ruby 1.8.6...

@rosenfeld

Maybe Rspec could deprecate the support for 1.8.6 some day...

@dchelimsky
Owner

@rosenfeld we didn't add instance_exec to every object, only those that need it in rspec. We could do the same here:

{}.extend(Tap).tap do |options|
@rosenfeld

Take a look on my last commit. If that is ok for you, let me know and I'll rebase it ('rebase -i') using the last commit as a fix for the first one. I didn't do it yet because maybe some people will prefer to compare both for now...

@rosenfeld

@dchemlimsky Ok, I get it. Then I see no problem with that approach.

@rosenfeld

So, what do you prefer, my last commit approach or implementing the Tap module and using that approach?

@rosenfeld

Sorry, I though I have sent this commit before, but I haven't even commited it (I didn't see the failing messages :) )

@dchelimsky
Owner

I like the 2nd commit because it's sufficiently small to grok the intent of the one-liner, though I'd prefer w/ parentheses.

I, personally, don't find the 1st commit easier to read than the code that is already htere. It's pretty dense and doesn't really tell the story as clearly the current code does. I'd be there's a middle ground that would keep the intent clear (or make it more so) and tighten it up a bit. Feel free to experiment.

@dchelimsky
Owner

@rosenfeld - the previous comment was in reference to 0499b49 (written before I saw 8222d87)

@rosenfeld

Sorry, I'm a bit confused. Let me see if I understand you. I'm considering 3 commits:

1 - refactoring of parse_options with tap
2 - refactoring of command_line options
3 - refactoring of parse_options without tap

So, are you saying that you would rather prefer the below?

    @command_line_options ||= Parser.parse!(@args).merge(:files_or_directories_to_run => @args)

And what about the last commit?

@dchelimsky
Owner

I think something like this would be more clear:

@options ||= [file_options, command_line_options, env_options].inject {|merged, o| merged.merge(o)}

Then a new file_options method could handle the custom vs local/global files. WDYT?

@dchelimsky
Owner

Yeah - we keep typing over each other :) This convo might be better served in IRC, but I need to sign off now. Maybe catch up later or tomorrow if you're still confused about my meaning.

@rosenfeld

Yeah, maybe Github should use some Comet or Websocket implementation for transforming this on a chat-like system... :)

I think the last commit is what you've talked about...

@rosenfeld

Actually, the last one seems to be clearer... :)

@myronmarston

@markiz: We don't want to discontinue 1.8.6/1.9.1 support yet because it prevents gem authors who use RSpec for testing from supporting those ruby versions. As a gem author, I'd want to deliberately decide when to stop supporting a ruby version, and not be forced to do so because my test framework of choice has stopped supporting the ruby version.

I expect that RSpec will drop support at some future point, but we're not there yet.

@dchelimsky
Owner

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 6 additions and 20 deletions.
  1. +6 −20 lib/rspec/core/configuration_options.rb
View
26 lib/rspec/core/configuration_options.rb
@@ -57,35 +57,21 @@ def drb_argv
end
def parse_options
- @options = begin
- options_to_merge = []
- if custom_options_file
- options_to_merge << custom_options
- else
- options_to_merge << global_options
- options_to_merge << local_options
- end
- options_to_merge << command_line_options
- options_to_merge << env_options
-
- options_to_merge.inject do |merged, options|
- merged.merge(options)
- end
- end
+ @options ||= [file_options, command_line_options, env_options].inject {|merged, o| merged.merge o}
end
private
+ def file_options
+ custom_options_file ? custom_options : global_options.merge(local_options)
+ end
+
def env_options
ENV["SPEC_OPTS"] ? Parser.parse!(ENV["SPEC_OPTS"].split) : {}
end
def command_line_options
- @command_line_options ||= begin
- options = Parser.parse!(@args)
- options[:files_or_directories_to_run] = @args
- options
- end
+ @command_line_options ||= Parser.parse!(@args).merge :files_or_directories_to_run => @args
end
def custom_options
Something went wrong with that request. Please try again.