Comment out and document the generated config #877

Merged
merged 1 commit into from Sep 3, 2013

5 participants

@sorentwo

All of the settings within the config file are slated to become the
default for 3.0. In the meantime comment them out of the generated
configuration and provide explanitory comments, hopefully to reduce any
confusion for people who are starting new projects.

As discussed in rspec/rspec-core#796

@JonRowe
RSpec member

If I'm reading this correctly, this will revert the generated config settings from the current release until RSpec 3 is released?

I.e. we need to wait until we're ready for master to be RSpec 3

@sorentwo

@JonRowe I think the implementation in rspec/rspec-core#796 is the more likely way to go. That more logically follows what will be established defaults in RSpec 3.

@JonRowe
RSpec member

My point was that until we make those items the default in RSpec 3 they should stay in the generated config file. I am in favour overall of #796, it's the introduction point I'm quibbling.

@samphippen
RSpec member

@myronmarston WDYT about this, I'm mostly in favour, but I have some code review notes I'd like to leave if you're generally in favour of the change.

@sorentwo are you still around to carry this one over the finish line?

@sorentwo

@samphippen I'm still around. Post the reviews and I'll apply whatever is needed.

@samphippen samphippen commented on the diff Sep 1, 2013
lib/rspec/core/project_initializer.rb
@@ -35,9 +35,9 @@ def create_dot_rspec_file
def create_spec_helper_file
if File.exist?('spec/spec_helper.rb')
- report_exists("spec/spec_helper.rb")
+ report_exists('spec/spec_helper.rb')
@samphippen
RSpec member
samphippen added a line comment Sep 1, 2013

@sorentwo I'm not sure why you churned these lines, was there a reason to switch the quotes type?

@sorentwo
sorentwo added a line comment Sep 1, 2013

Personal preference aside, it was done for consistency. All of the other quotes are single, those were the only two instances of double quotes.

@samphippen
RSpec member
samphippen added a line comment Sep 1, 2013

Ok that's fine, as long as there's a reason. I'd prefer them to b e consistent within the file. So you can leave it as is 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samphippen samphippen commented on the diff Sep 1, 2013
lib/rspec/core/project_initializer.rb
else
- report_creating("spec/spec_helper.rb")
+ report_creating('spec/spec_helper.rb')
@samphippen
RSpec member
samphippen added a line comment Sep 1, 2013

@sorentwo same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@samphippen samphippen commented on an outdated diff Sep 1, 2013
lib/rspec/core/project_initializer.rb
@@ -48,15 +48,29 @@ def create_spec_helper_file
#
# See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
RSpec.configure do |config|
- config.treat_symbols_as_metadata_keys_with_true_values = true
- config.run_all_when_everything_filtered = true
- config.filter_run :focus
+ # Specify metadata using just symbols. Each symbol used will effectively be
+ # treated as a `true` value, allowing easier filtering when running specs.
+ # it 'can be filtered using simple metadata', :fast, :simple do
+ # end
+ #
+ # rspec -t fast
+ # config.treat_symbols_as_metadata_keys_with_true_values = true
@samphippen
RSpec member
samphippen added a line comment Sep 1, 2013

@sorentwo see this pull request: #1006, we won't be merging this for anything other than RSpec 3.0, so you don't need to talk about treat_symbols_as_metadata_keys_with_true_values, it's always enabled.

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

@sorentwo WDYT about changing the whitespace on the comments slightly so that they're formed like this:

    # some paragraph
    # about the thing
    #code

that is no space between the # and the code, but space between the # and the plaintext docs?

@samphippen
RSpec member

@sorentwo you'll also need to rebase this against master. I'm in favour of this change but would like @myronmarston to provide thoughts before we merge it.

Thanks

@sorentwo

@samphippen That formatting makes sense. I'll change it after the rebase.

@sorentwo

@samphippen Rebased and modified as amended.

@coveralls

Coverage Status

Coverage increased (+14.39%) when pulling f57f3f1 on sorentwo:document-generated-config into ea43215 on rspec:master.

@samphippen
RSpec member

@myronmarston thoughts on this? I'll merge and changelog entry if you think it's good :)

@myronmarston myronmarston and 1 other commented on an outdated diff Sep 3, 2013
lib/rspec/core/project_initializer.rb
@@ -44,14 +44,21 @@ def create_spec_helper_file
#
# See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration
RSpec.configure do |config|
- config.run_all_when_everything_filtered = true
- config.filter_run :focus
+ # Run all specs when none match the provided filter. This works well in
+ # conjunction with `config.filter_run :focus`, as it will run the entire
+ # suite when no specs have `:filter` metadata.
+ #config.run_all_when_everything_filtered = true
+
+ # Limit the spec run to only specs with the focus metadata. If no specs have
+ # the filtering metadata and `run_all_when_everything_filtered = true` then
+ # all specs will run.
+ #config.filter_run :focus
@myronmarston
RSpec member
myronmarston added a line comment Sep 3, 2013

IMO, it makes more sense for filter_run to come before run_all_when_everything_filtered, since run_all_when_everything_filtered is essentially a modification of the behavior of filter_run.

@sorentwo
sorentwo added a line comment Sep 3, 2013

The dedscription of filter_run includes a reference to run_all_when_everything_filtered. I believe that is why it is in that order. Internally it may be a modification of the behavior, but to newcomers that wouldn't be obvious.

I'm all for changing the order if you feel it is necessary though.

@myronmarston
RSpec member
myronmarston added a line comment Sep 3, 2013

Please do change it. Thanks!

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

One comment, but looks good otherwise.

@sorentwo sorentwo Comment out and document the generated config
All of the settings within the config file are slated to become the
default for 3.0. In the meantime comment them out of the generated
configuration and provide explanitory comments, hopefully to reduce any
confusion for people who are starting new projects.
77a3ec7
@sorentwo

@myronmarston Order switched, all set.

@coveralls

Coverage Status

Coverage increased (+14.39%) when pulling 77a3ec7 on sorentwo:document-generated-config into ea43215 on rspec:master.

@myronmarston
RSpec member

Thanks, I'll leave @samphippen to merge this then.

@samphippen samphippen merged commit 818a145 into rspec:master Sep 3, 2013

1 check passed

Details default The Travis CI build passed
@samphippen samphippen added a commit that referenced this pull request Sep 3, 2013
@samphippen samphippen Add a changelog entry for #877. 120fb71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment