Expose configuration options #834

Merged
merged 13 commits into from Apr 18, 2013

Projects

None yet

4 participants

@JonRowe
RSpec member

An alternate to #832 this solves #732 by exposing readers for a few of the configuration options that don't already have them.

@soulcutter
RSpec member

Just a heads up, there will be a conflict with #867 specifically the requires= / requires bit.

Other than that, I think this looks good.

@myronmarston
RSpec member

Oh yeah, I still need to review this...will try to get to it soon :).

@soulcutter
RSpec member

Closing in favor of #874 (which is just this PR merged into master)

@soulcutter soulcutter closed this Apr 17, 2013
@JonRowe
RSpec member

I prefer rebasing and force pushing over merging in another branch, as this preserves the discussion centered around the PR, and I only havent rebased it because I'm still awaiting @myronmarston's feedback...

@soulcutter
RSpec member

I can force push to your repo? I wasn't sure how to deal with that.

Sorry if I stepped on toes this way - my thinking was I'd fix this PR since I broke it (merge-conflict-wise).

FWIW this change gets 👍 to merge into master from me, but also didn't want to step on Myron's toes since this was on his radar.

@JonRowe
RSpec member

I just meant it was awaiting more feedback, then I was going to rebase it.

You can't push to my repo unless I add you all as collaborators, which actually might be reasonable.

@myronmarston
RSpec member

I prefer keeping the extra merge commit out as well, so I'll comment here.

@myronmarston myronmarston reopened this Apr 17, 2013
@myronmarston myronmarston and 1 other commented on an outdated diff Apr 17, 2013
lib/rspec/core/configuration.rb
@@ -423,6 +424,9 @@ def expect_with(*frameworks)
@expectation_frameworks.push(*modules)
end
+ def full_backtrace
+ @backtrace_clean_patterns.empty?
+ end
def full_backtrace=(true_or_false)
@myronmarston
myronmarston Apr 17, 2013

Please put blank lines between method defs -- I find that it helps readability a lot.

@JonRowe
JonRowe Apr 17, 2013

Done, sorry, I suck at that.

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 17, 2013
lib/rspec/core/configuration.rb
@@ -455,10 +459,17 @@ def color=(bool)
def libs=(libs)
libs.map {|lib| $LOAD_PATH.unshift lib}
end
+ def libs
+ $LOAD_PATH
+ end
@myronmarston
myronmarston Apr 17, 2013

I'm a little concerned that this isn't symmetric with libs= -- this will return all load paths, not just the ones the user has added using libs=. I think that these reader/writer methods should be symmetric; otherwise it gets confusing.

@JonRowe
JonRowe Apr 17, 2013

Well, I could argue that I did this because the paths can be mutated from anywhere prior too... However I will defer and make this capture only the path's we've configured.

@myronmarston
myronmarston Apr 17, 2013

Well, I could argue that I did this because the paths can be mutated from anywhere prior too.

That's another reason I think this should only return the load paths that have been added through rspec config: because if the user wants all load paths they can already get it via $LOAD_PATH. However, they don't have a way to get the load paths that were configured via rspec, and that, to me, is the value proposition of having this method at all. (If it's just going to return $LOAD_PATH, I'd just encourage users to use $LOAD_PATH directly.)

@JonRowe
JonRowe Apr 17, 2013

Heh the @github notification email couldn't cope with the outdated diff there, oh well, I've changed this in my most recent push anyhow.

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 17, 2013
lib/rspec/core/configuration.rb
@@ -480,15 +491,24 @@ def debug=(bool)
EOM
end
end
+ def debug
+ !!defined?(Debugger)
+ end
@myronmarston
myronmarston Apr 17, 2013

FWIW, I'm planning on removing the debug option in rspec 3...but it's fine to have this for now.

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 17, 2013
lib/rspec/core/configuration.rb
# Run examples defined on `line_numbers` in all files to run.
def line_numbers=(line_numbers)
filter_run :line_numbers => line_numbers.map{|l| l.to_i}
end
+ def line_numbers
+ filter.has_key? :line_numbers
+ end
@myronmarston
myronmarston Apr 17, 2013

Again, this isn't symmetric -- line_numbers= accepts an array of line numbers, but this returns a boolean...that seems odd.

@JonRowe
JonRowe Apr 17, 2013

This one makes the most sense to return directly, so done.

@myronmarston myronmarston and 1 other commented on an outdated diff Apr 17, 2013
lib/rspec/core/configuration.rb
def full_description=(description)
filter_run :full_description => Regexp.union(*Array(description).map {|d| Regexp.new(d) })
end
+ def full_description
+ filter.has_key? :full_description
+ end
@myronmarston
myronmarston Apr 17, 2013

This isn't symmetric either -- any reason not to have this return the assigned full description rather than just a boolean?

@JonRowe
JonRowe Apr 17, 2013

The thinking behind these non symmetrical readers was that they were filters, so they indicated wether the filters were on or not, (this thinking was caused by my reading of the specs, which are rather... oddly... structured when it comes to these 3 methods... )

@myronmarston
RSpec member

Woops, I accidentally got my chrome tabs confused and left a couple comments on @soulcutter's PR. I might move them over later but I don't have the time right now.

@myronmarston myronmarston commented on the diff Apr 18, 2013
lib/rspec/core/backtrace_cleaner.rb
@@ -32,6 +32,10 @@ def full_backtrace=(true_or_false)
@exclusion_patterns = true_or_false ? [] : DEFAULT_EXCLUSION_PATTERNS.dup
end
+ def full_backtrace
+ @exclusion_patterns.empty?
+ end
+
@myronmarston
myronmarston Apr 18, 2013

Given that this is a boolean predicate, maybe it should be full_backtrace?.

@myronmarston myronmarston commented on the diff Apr 18, 2013
lib/rspec/core/configuration.rb
@@ -462,6 +466,10 @@ def expect_with(*frameworks)
@expectation_frameworks.push(*modules)
end
+ def full_backtrace
+ @backtrace_cleaner.full_backtrace
+ end
@myronmarston
myronmarston Apr 18, 2013

Same here -- I think it would be more idiomatic as full_backtrace?.

@myronmarston myronmarston commented on the diff Apr 18, 2013
lib/rspec/core/configuration.rb
@@ -523,15 +534,27 @@ def debug=(bool)
end
end
+ def debug
+ !!defined?(Debugger)
+ end
@myronmarston
myronmarston Apr 18, 2013

Maybe here, too? (debug?)

@myronmarston myronmarston commented on the diff Apr 18, 2013
lib/rspec/core/configuration.rb
def full_description=(description)
filter_run :full_description => Regexp.union(*Array(description).map {|d| Regexp.new(d) })
end
+ def full_description
+ filter.fetch :full_description, false
+ end
@myronmarston
myronmarston Apr 18, 2013

Do you think nil makes more sense here than false? Usually false is paired with true, and other wise nil is used...

@myronmarston myronmarston merged commit de6723d into rspec:master Apr 18, 2013

1 check passed

Details default The Travis build passed
@myronmarston
RSpec member

I left a couple more minor comments, (sorry, some of this stuf I didn't notice until just now), but went ahead and merged because this is definitely a big improvement. If the other comments are deemed worthy of addressing it can always be done post-merge.

Can you add a change log entry, though?

Thanks!

@JonRowe
RSpec member

Added changelog over on #876

@JonRowe JonRowe deleted the JonRowe:alternate_expose_config_options_to_config branch Apr 18, 2013
@dchelimsky
RSpec member

I just noticed this because I was working on BacktraceCleaner and wondered where full_backtrace? gets used and it turns out it doesn't (at least I couldn't find any use of it in rspec-core). What's the motivation to add these config properties?

RSpec member

PS I realize this is 5 months old :)

RSpec member

I believe the motivation for this was solving #732. (See #832 and #834 for further discussion).

A user wanted to be able to do a conditional thing based on if the --backtrace and there wasn't a public API available for that...so @JonRowe added full_backtrace?.

RSpec member

Ha! Turns out I recommended it!

Thx @myronmarston, and thx @JonRowe for implementing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment