Expose ConfigurationOptions to Configuration

[JonRowe]

Add in a way to expose configuration options into configuration, as a suggested implementation for #732

[cupakromer]

Is there a reason to use cmdline_ as a prefix instead of exposing the option name directly? It seems the intent is to get access to some things passed in from the cli. As a user, I wouldn't care under normal circumstances if the option was initially set in the cli or some where else, as long as I had access to it's current value.

[JonRowe]

Yes. It's to indicate that they are not actual Configuration options, but the options passed into the runner, they come from the command line flags or via the runner itself (in the case of automation tools). They're only really for reference use, as you can't change them, additionally, I didn't want to risk conflict with current Configuration options.

I'd be open to a different prefix, or removing the prefix entirely if @dchelimsky / @myronmarston preferred.

[dchelimsky]

I like making the distinction between options defined on RSpec.configuration and those defined elsewhere, but those options can come from the command line, a file, or even an environment variable, so I like the idea of a prefix, but cmdline is a bit misleading. I don't have any good ideas but if/when I do I'll post them here.

[JonRowe]

I didn't know what else to call them, but I agree that cmdline_ is potentially misleading... I was hoping someone would have a better suggestion (next time I'll mention those sort of things up front in the PR ;) )

[JonRowe]

How about just option_ to indicate it's an option that's been configured elsewhere?

[myronmarston]

Rather than exposing them directly off of the configuration object, what do you think about exposing a single object off of the configuration object (called something like cli_options), and then exposing each option off of that, w/o a prefix?

[myronmarston]

FWIW, I always prefer define_method over eval'ing a string like this. Tenderlove recently wrote up a summary comparison of the two approaches including benchmarks:

http://tenderlovemaking.com/2013/03/03/dynamic_method_definitions.html

We haven't always followed this in the rspec code base, both because we've never really talked about it as a team (and I'm not sure that others share my preference for define_method) and also because we've been forced to use the eval approach on occasion due to the fact that 1.8.6 (and earlier) didn't allow a define_method-defined method to accept a block...but moving forward, I'd like to use define_method unless we have a specific reason not to.

[JonRowe]

Totally, I'd be happy to switch it, but I was following the convention of the other methods.

[JonRowe]

I thought about that but @dchelimsky's comment in #732

I'd rather move toward ensuring that all of the options are exposed on the Configuration than
exposing another object.

convinced me otherwise.

[myronmarston]

Sounds like I need to read more of the background and not just be a drive-by commenter :).

[myronmarston]

@dchelimsky -- can you speak to your reasons for the statement @JonRowe quoted above? I prefer a separate object; to me, programmatically accessing the passed CLI options has quite different semantics from normal RSpec::Configuration options -- for example, they are read-only (because you can't change what the passed CLI options were once the program has begun...). I also feel like it's a cleaner approach then a common method prefix--in fact, the discussion of the method prefix is what suggested to me that we need a separate object for this. I'm sure you have totally valid reasons for the preference you expressed before but I can't think of any reasons to favor that approach at the moment.

[dchelimsky]

Several (but not all) of the options can be set in a pre-process scope (e.g. external files like .rspec and/or ENV vars), and in RSpec.configure. I think it would add confusion to expose to end users e.g. RSpec.configuration.fail_fast and RSpec.configuration.pre_process_options.fail_fast.

[cupakromer]

@dchelimsky that eloquently put into words what I was thinking in my first comment.

I read the initial issue as the need to have access to some of these read only settings (no matter where they were set). However, if there is a perfectly go alternative already in RSpec.configuration, say the list of tests to run, then use what is already in RSpec.configuration which has already been properly processed for consumption.

[JonRowe]

Ok, so which should we disregard? Is there harm in exposing these under a prefix for reference purposes?

[myronmarston]

I think it would add confusion to expose to end users e.g. RSpec.configuration.fail_fast and RSpec.configuration.pre_process_options.fail_fast.

But how is that any less confusing than RSpec.configuration.fail_fast and RSpec.configuration.pre_process_options.fail_fast? I don't see how a prefix is any better in this regard to their being separate object.

Thinking about this some more...I think it's pretty rare that a user needs to access the command line options (consider that #732 is the first time that a user has ever requested it, as far as I know). The approaches we're talking about here add some maintenance cost in that every time we add a new command line option, we'd have to make sure there's a corresponding method added to RSpec::Configuration (or the the object returned by RSpec::Configuration#cli_options if we go that route). Given the rarity of this need, I'd prefer not to add to our maintenance cost. I think a better approach, which doesn't add to our ongoing maintenance cost, and also removes the potential confusion of having multiple config methods named the same thing, is to have RSpec::Configuration#cli_options returned the unprocessed options using a ruby primitive (either a raw string or a hash). The end user code that consumes the options could then be something like if RSpec.configuration.cli_options.include?('--profile').... This has a nice side benefit of not requiring the user to reference API docs to see which command line flags correspond to which methods on a config object; The --some-random-option CLI option would come through in the returned object as --some-random-option -- no translation or API really needed!

[dchelimsky]

Me: I think it would add confusion to expose to end users e.g. RSpec.configuration.fail_fast and RSpec.configuration.pre_process_options.fail_fast.

You: But how is that any less confusing than RSpec.configuration.fail_fast and RSpec.configuration.pre_process_options.fail_fast?

Me: Huh?

re: exposing CLI args via cli_options, what does RSpec.configuration.cli_options.include?('--color') return when .rspec contains --color and the CLI contains --no-color?

[myronmarston]

Me: Huh?

Sorry, I completely failed to make my point.

The point I was trying to make is that while you are correct that having both RSpec.configuration.cli_options.fail_fast and RSpec.configuration.fail_fast could be confusing to an end user, I think it would be equally confusing to have both RSpec.configuration.cli_options_fail_fast and RSpec.configuration.fail_fast. IMO, having either a method prefix or an "object prefix" (for lack of a better term) both have the potential to cause equal confusion...but I find the "object prefix" solution to have higher cohesion. These CLI options belong together, and it seems odd (to me) to mix them into another object, but then try to differentiate them by using a common method prefix.

re: exposing CLI args via cli_options, what does RSpec.configuration.cli_options.include?('--color') return when .rspec contains --color and the CLI contains --no-color?

I've been thinking this would only be the options passed at the command line, so it would be --no-color. As I see it, the options go through multiple processing phases:

• It starts with the raw CLI options
• This gets merged with .rspec options
• These get applied to the RSpec.configuration object, and the user can override them further.

RSpec.configuration contains the fully-processed current configuration options. I can see the usefulness of seeing exactly what got passed at the command line (e.g. before any more processing is done), and having access to the fully processed configuration options is quite useful, but it seems odd to me to have this feature return the partially-processed config options created from the first two phases.

If nothing else, this conversation reveals that there are some semantics of this feature that we need to nail down.

[dchelimsky]

As I see it, the options go through multiple processing phases:

It starts with the raw CLI options
This gets merged with .rspec options
These get applied to the RSpec.configuration object, and the user can override them further.

It's more like this:

• ENV["SPEC_OPTS"] options get merged over
• CLI options, which get merged over
• if custom options file
  • custom options file, which get merged over
• else
  • ./.rspec-local (not in VCS), which get merged over
  • ./.rspec (in VCS), which get merged over
  • ~/.rspec (not in VCS), which get merged over
• RSpec.configure options

See https://github.com/rspec/rspec-core/blob/master/lib/rspec/core/configuration_options.rb#L76-L82

That the ENV options take precedence over the CLI is a legacy support issue: #276

In terms of the issue we're trying to address, if a user wishes to modify options in RSpec.configure based on CLI options, he/she is just as likely to want to be able to drive that from a user-specific options file (project-local or global), so I think exposing options at any level beyond the final merge is not only opening a rather ugly can of worms, it would also limit our options to simplify things down the road.

If, in spite of that, there is a decision to press forward, I think we should go all in and offer up an ordered source map e.g.

RSpec.configuration.source_for :color
# => [ [ ".rspec", "--no-color" ], [ "/Users/david/.rspec", "--color" ], [ "RSpec.configuration", "true" ] ]

# This would be nicer as an ordered hash, but we'd have to employ special handling for Ruby 1.8 e.g. `ActiveSupport::OrderedHash`.

I could see that being useful as a debugging tool when trying to understand option-input precedence, but how often does that need come up?

[myronmarston]

Thanks for writing that up, @dchelimsky -- the config system is one of the parts of rspec I've worked in the least. I had forgotten how man layers it has.

The ordered source map idea looks nice, but it sounds like a lot of effort for something that would be rarely used.

Regardless of what direction we go (or if we do anything at all), I want to make sure that we actually wind up addressing @jasonkarns' issue he was dealing with in #732. (Jason, we'd value your feedback here!)

Revisiting my earlier idea to expose the CLI args string (which was clearly naive), would it work to expose the merged options as a hash?

[JonRowe]

@myronmarston did you check out #834 ? I feel it might be a better appraoch to solving the simple cases like @jasonkarns' #732 without getting into the complexity @dchelimsky suggested, which I feel might be overkill...

[dchelimsky]

FWIW I agree it would be complete overkill :) I just think dipping a toe into a deep river makes no sense either.

I think @JonRowe's solution in #834 is the right way to go here because it solves the problem without introducing a new concept.

[jasonkarns]

I also like @JonRowe's solution in #834. That solves my initial issue easily. Originally, all I had wanted to do was to set our Logger level to DEBUG when RSpec was in debugging mode. It didn't matter to me whether RSpec debug mode was triggered via ENV, CLI, or .rspec file. All I was interested in was the final option state after all option parsing/merging was complete. I would hazard a guess that this hits the 80% use-case anytime one wants access to a configuration option.

[myronmarston]

Sounds good. Let's move our discussion to #834. @JonRowe -- I'm heading to bed but will try to do a code review of it tomorrow.