Achieve 100% doc coverage [DONE] #1362

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

yelled3 commented Mar 2, 2014

see: #1334

currently:

Files:          48
Modules:        31 (   21 undocumented)
Classes:        50 (   27 undocumented)
Constants:      27 (   25 undocumented)
Methods:       388 (  202 undocumented)
 44.56% documented

DONE:

yard stats --list-undoc
Files:          30
Modules:        17 (    0 undocumented)
Classes:        25 (    0 undocumented)
Constants:       3 (    0 undocumented)
Methods:       142 (    0 undocumented)
 100.00% documented

Owner

JonRowe commented Mar 2, 2014

Thanks for getting started on this, but these methods are already documented as private, so if they're affecting the stats I think it makes more sense to switch them to @private so they're hidden entirely...

Contributor

yelled3 commented Mar 2, 2014

righto - just saw @xaviershay comment in:
rspec/rspec-mocks#585

@yelled3 yelled3 and 1 other commented on an outdated diff Mar 2, 2014

lib/rspec/core/world.rb
@@ -40,6 +40,8 @@ def reset
end
# @api private
+ #
+ # @return [RSpec::Core::FilterManager]
def filter_manager
@configuration.filter_manager
@yelled3

yelled3 Mar 2, 2014

Contributor

not related to this PR, but I'm not sure why so many delegation methods - should be simply

delegate :filter_manager, :ordering_registry, :inclusion_filter, :exclusion_filter, :configure_group, :reporter, :run_all_when_everything_filtered?, 
   :to => :configuration

see:
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/module/delegation.rb

@JonRowe

JonRowe Mar 2, 2014

Owner

RSpec has a policy of using as few dependencies as possible, and we even prefer not to load the std library when possible as it could cause a false positive (your test passes because we've loaded a library, but your code fails in production because you don't load it), thus there's no way in hell we'd use ActiveSupport.

Contributor

yelled3 commented Mar 3, 2014

@JonRowe can you please confirm I'm on the right track here?

@api private VS @Private is really confusing...

@xaviershay xaviershay commented on an outdated diff Mar 3, 2014

lib/rspec/core/pending.rb
module Pending
+ # @private
@xaviershay

xaviershay Mar 3, 2014

Member

This one should be public.

"Raised in the middle of an example to indicate that it should be marked as skipped."

@xaviershay xaviershay commented on the diff Mar 3, 2014

lib/rspec/core/reporter.rb
@@ -50,46 +52,56 @@ def report(expected_example_count)
end
end
+ # @private
@xaviershay

xaviershay Mar 3, 2014

Member

@JonRowe are these events documented elsewhere? Or do you think it makes sense to document them here?

@JonRowe

JonRowe Mar 4, 2014

Owner

The base formatter documents what notifications are acceptable on a formatter, but the reporter api is semi private?

@xaviershay xaviershay and 1 other commented on an outdated diff Mar 3, 2014

lib/rspec/core/runner.rb
@@ -1,5 +1,6 @@
module RSpec
module Core
+ # Rspec runner
@xaviershay

xaviershay Mar 3, 2014

Member

Rspec -> RSpec.

If this class is public, we should document here who should be in the business of using it and when. (Extensions? I'm not sure.) The comment as stands is just a copy of the class name.

@yelled3

yelled3 Mar 5, 2014

Contributor

seems like it's only used internally...

@xaviershay xaviershay commented on an outdated diff Mar 3, 2014

lib/rspec/core/shared_example_group/collection.rb
@@ -1,12 +1,16 @@
module RSpec
module Core
module SharedExampleGroup
+ # @api private
+ #
+ # Shared example groups collection
@xaviershay

xaviershay Mar 3, 2014

Member

Can we just mark this one @private? Comment isn't adding value.

@xaviershay xaviershay commented on an outdated diff Mar 3, 2014

lib/rspec/core/version.rb
module Version
+ # Current version string
@xaviershay

xaviershay Mar 3, 2014

Member

nit: see the comment I added to the mocks version of this constant. Documenting the format is useful, I think.

Member

xaviershay commented Mar 3, 2014

@api private VS @Private is really confusing...

yeah I agree. I default to @private. Something has to have a good reason for showing up in the documentation to mark it @api private, I think. (though I don't feel like the core team has really hashed this out and decided on a policy yet - or at least we're not all sure of the same things)

@JonRowe JonRowe commented on the diff Mar 4, 2014

lib/rspec/core/formatters.rb
@@ -58,6 +58,7 @@ module RSpec::Core::Formatters
autoload :ProgressFormatter, 'rspec/core/formatters/progress_formatter'
autoload :JsonFormatter, 'rspec/core/formatters/json_formatter'
+ # Register the formatter class
@JonRowe

JonRowe Mar 4, 2014

Owner

This is a public method, it'd be good to add the params in.

@JonRowe JonRowe commented on an outdated diff Mar 4, 2014

lib/rspec/core/world.rb
@@ -39,7 +39,9 @@ def reset
SharedExampleGroup.registry.clear
end
- # @api private
+ # @private
+ #
+ # @return [RSpec::Core::FilterManager]
@JonRowe

JonRowe Mar 4, 2014

Owner

As this is @private this does nothing surely.

@JonRowe JonRowe commented on an outdated diff Mar 4, 2014

lib/rspec/core/world.rb
@@ -52,17 +54,21 @@ def register(example_group)
example_group
end
- # @api private
+ # @private
+ #
+ # @return [RSpec::Core::FilterManager::InclusionFilterHash] if none has been set, returns an empty hash.

@JonRowe JonRowe commented on an outdated diff Mar 4, 2014

lib/rspec/core/world.rb
def inclusion_filter
@configuration.inclusion_filter
end
- # @api private
+ # @private
+ #
+ # @return [RSpec::Core::FilterManager::ExclusionFilterHash] if none has been set, returns an empty hash.

@JonRowe JonRowe commented on an outdated diff Mar 4, 2014

lib/rspec/core/world.rb
@@ -86,7 +92,11 @@ def preceding_declaration_line(filter_line)
end
end
- # @api private
+ # @private
+ #
+ # @return [RSpec::Core::Reporter]
+ #
+ # Get the configured reporter
@JonRowe

JonRowe Mar 4, 2014

Owner

Ditto and unnecessary.

@JonRowe JonRowe commented on an outdated diff Mar 4, 2014

lib/rspec/core/world.rb
@@ -130,7 +140,9 @@ def announce_filters
end
end
- # @api private
+ # @private
+ #
+ # @return [String] A message displayed when all examples where filtered out
Contributor

yelled3 commented Mar 5, 2014

@JonRowe @xaviershay ALL DONE!
please review.

BTW, something's wrong with the CI:
https://travis-ci.org/rspec/rspec-core/jobs/20146423
hope it's not me :-)

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -151,6 +152,9 @@ def expose_dsl_globally=(value)
# Default: `$stdout`.
# Also known as `output` and `out`
define_reader :output_stream
+
+ # Set the output stream for reporter
+ # Default $stdout
@JonRowe

JonRowe Mar 5, 2014

Owner

This could do with an @attr [IO] defaults to $stdout to better describe the param

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -166,6 +170,7 @@ def output_stream=(value)
# Load files matching this pattern (default: `'**/*_spec.rb'`)
add_setting :pattern, :alias_with => :filename_pattern
+ # Set pattern matching files to load
def pattern= value
@JonRowe

JonRowe Mar 5, 2014

Owner

Similarly here adding in the rdoc for the expected param would help.

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -355,6 +360,7 @@ def backtrace_exclusion_patterns
@backtrace_formatter.exclusion_patterns
end
+ # Set Regexps used to exclude lines in backtrace

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -371,11 +377,12 @@ def backtrace_inclusion_patterns
@backtrace_formatter.inclusion_patterns
end
+ # Set Regexps used to include lines in backtrace
@JonRowe

JonRowe Mar 5, 2014

Owner

And I'd use "Set regular expressions used" rather than an acronym here.

@JonRowe JonRowe commented on the diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -506,14 +513,19 @@ def expect_with(*frameworks)
@expectation_frameworks.push(*modules)
end
+ # Check if full backtrace is enabled
@JonRowe

JonRowe Mar 5, 2014

Owner

I'd state the return here

@JonRowe JonRowe commented on the diff Mar 5, 2014

lib/rspec/core/configuration.rb
def line_numbers
filter.fetch(:line_numbers,[])
end
+ # Run examples matching on `description` in all files to run.
@JonRowe

JonRowe Mar 5, 2014

Owner

Adding the attr here would help

@JonRowe

JonRowe Mar 11, 2014

Owner

Did you remove the attr here? How about @param [String] apply filter to full description?

@JonRowe JonRowe commented on the diff Mar 5, 2014

lib/rspec/core/drb_command_line.rb
class DRbCommandLine
def initialize(options)
@options = options
end
+ # The DRB port
@JonRowe

JonRowe Mar 5, 2014

Owner

A return would be good here

@JonRowe

JonRowe Mar 6, 2014

Owner

Still missing a return here, @return [Fixnum] The port to use for DRB

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/configuration.rb
@@ -1037,6 +1058,7 @@ def warnings= value
$VERBOSE = !!value
end
+ # Get Ruby warnings
@JonRowe

JonRowe Mar 5, 2014

Owner

Adding a return here would help, also this is slightly misleading, as it's returning the verbosity, not the warnings itself. How about @return [Fixnum] Ruby's warning level (I think $VERBOSE is a Fixnum or nil)

@JonRowe JonRowe commented on an outdated diff Mar 5, 2014

lib/rspec/core/formatters/base_formatter.rb
@@ -73,7 +73,7 @@ def example_group_started(notification)
#
# Invoked at the beginning of the execution of each example.
#
- # @param example instance of subclass of `RSpec::Core::ExampleGroup`
+ # @param notification instance of subclass of `RSpec::Core::ExampleGroup`
@JonRowe

JonRowe Mar 5, 2014

Owner

the rest of this is inaccurate too, notification is not an example

Owner

JonRowe commented Mar 5, 2014

Getting there! I left a few more comments, we'll also need all this squashed before merging.

@JonRowe JonRowe commented on an outdated diff Mar 6, 2014

lib/rspec/core/formatters.rb
@@ -58,6 +58,11 @@ module RSpec::Core::Formatters
autoload :ProgressFormatter, 'rspec/core/formatters/progress_formatter'
autoload :JsonFormatter, 'rspec/core/formatters/json_formatter'
+ # Register the formatter class
+ # @param [Class] formatter_class formatter class to register
+ # @param [args] notifications one or more notifications to be registered to the specified formatter
@JonRowe

JonRowe Mar 6, 2014

Owner

The param in the [] is supposed to the type, but args is a splat, I'm not sure how to doc that correctly but this is wrong :) /cc @myronmarston

@JonRowe JonRowe commented on an outdated diff Mar 6, 2014

lib/rspec/core/formatters/base_text_formatter.rb
@@ -33,8 +33,7 @@ def dump_failures(notification)
#
# Colorizes the output red for failure, yellow for
# pending, and green otherwise.
- #
- # @param [String] string
+ # @param [String] summary
@JonRowe

JonRowe Mar 6, 2014

Owner

This isn't a string but a SummaryNotification

@JonRowe JonRowe commented on an outdated diff Mar 6, 2014

lib/rspec/core/metadata.rb
@@ -26,6 +26,9 @@ module Core
# @see Configuration#filter_run_excluding
class Metadata < Hash
+ # @api private
+ #
+ # @return [String] relative path to line
@JonRowe

JonRowe Mar 6, 2014

Owner

Might be worth mentioning the param here too

Contributor

yelled3 commented Mar 11, 2014

@JonRowe we good? can I squash?

@myronmarston myronmarston commented on an outdated diff Mar 11, 2014

lib/rspec/core/dsl.rb
@@ -59,6 +60,7 @@ def self.expose_globally!
@exposed_globally = true
end
+ # Removes the describe method to Module and the top level binding
@myronmarston

myronmarston Mar 11, 2014

Owner

This should remove Removes the describe method from ... not Removes the describe method to ....

@JonRowe JonRowe commented on an outdated diff Mar 11, 2014

lib/rspec/core/configuration.rb
@@ -506,14 +516,20 @@ def expect_with(*frameworks)
@expectation_frameworks.push(*modules)
end
+ # Check if full backtrace is enabled
+ # @return [Boolean] is backtrace enabled
@JonRowe

JonRowe Mar 11, 2014

Owner

This should be "is full backtrace enabled"

@JonRowe JonRowe commented on an outdated diff Mar 11, 2014

lib/rspec/core/configuration.rb
def full_backtrace?
@backtrace_formatter.full_backtrace?
end
+ # Toggle full backtrace
+ # @attr [Boolean] is full backtrace enabled
@JonRowe

JonRowe Mar 11, 2014

Owner

I think this should be "toggle full backtrace display"

@JonRowe JonRowe commented on an outdated diff Mar 11, 2014

lib/rspec/core/configuration.rb
@@ -522,6 +538,8 @@ def color(output=output_stream)
value_for(:color, @color)
end
+ # Toggle output color
+ # @attr [Boolean] is color enabled
@JonRowe

JonRowe Mar 11, 2014

Owner

Similarly "toggle color enabled"

Owner

JonRowe commented Mar 11, 2014

Looking better! I have a few more notes though, and @myronmarston provided one too. Note you can squash at anytime, we don't mind force pushing feature branches. Also note you'll need to rebase.

Contributor

yelled3 commented Mar 12, 2014

@JonRowe fixed the last notes + squashed all commits

JonRowe referenced this pull request Mar 12, 2014

Merged

Achieve 100% doc coverage #1383

JonRowe closed this in #1383 Mar 12, 2014

Owner

JonRowe commented Mar 12, 2014

Thanks for your hard work on this @yelled3, I took it over the line with a final rebase and a few more corrections :)

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