Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make compatible with latest RuboCop #448

Merged
merged 3 commits into from
Jan 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,8 @@ Style/FileName:
Exclude:
- 'spec/fixtures/utf-8.rb'

Style/TrailingComma:
EnforcedStyleForMultiline: 'comma'
Style/TrailingCommaInLiteral:
EnforcedStyleForMultiline: comma

Style/GuardClause:
Enabled: false
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ group :test do
gem "cucumber", "~> 2.0"
gem "phantomjs", "~> 1.9"
gem "poltergeist", "~> 1.1"
gem "rubocop", ">= 0.30"
gem "rubocop", "~> 0.36.0"
gem "test-unit", "~> 3.0"
end
end
Expand Down
16 changes: 12 additions & 4 deletions lib/simplecov.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,22 @@ def add_not_loaded_files(result)
# from cache using SimpleCov::ResultMerger if use_merging is activated (default)
#
def result
@result ||= SimpleCov::Result.new(add_not_loaded_files(Coverage.result)) if running
# Ensure the variable is defined to avoid ruby warnings
@result = nil unless defined?(@result)

# Collect our coverage result
if running && !result?
@result = SimpleCov::Result.new add_not_loaded_files(Coverage.result)
end

# If we're using merging of results, store the current result
# first, then merge the results and return those
if use_merging
SimpleCov::ResultMerger.store_result(@result) if @result
return SimpleCov::ResultMerger.merged_result
SimpleCov::ResultMerger.store_result(@result) if result?

SimpleCov::ResultMerger.merged_result
else
return @result if defined? @result
@result
end
ensure
self.running = false
Expand Down
2 changes: 1 addition & 1 deletion lib/simplecov/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def nocov_token(nocov_token = nil)
return @nocov_token if defined?(@nocov_token) && nocov_token.nil?
@nocov_token = (nocov_token || "nocov")
end
alias_method :skip_token, :nocov_token
alias skip_token nocov_token

#
# Returns the configured groups. Add groups using SimpleCov.add_group
Expand Down
19 changes: 10 additions & 9 deletions lib/simplecov/defaults.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,16 @@
# If we are in a different process than called start, don't interfere.
next if SimpleCov.pid != Process.pid

if $! # was an exception thrown?
# if it was a SystemExit, use the accompanying status
# otherwise set a non-zero status representing termination by some other exception
# (see github issue 41)
@exit_status = $!.is_a?(SystemExit) ? $!.status : SimpleCov::ExitCodes::EXCEPTION
else
# Store the exit status of the test run since it goes away after calling the at_exit proc...
@exit_status = SimpleCov::ExitCodes::SUCCESS
end
@exit_status = if $! # was an exception thrown?
# if it was a SystemExit, use the accompanying status
# otherwise set a non-zero status representing termination by
# some other exception (see github issue 41)
$!.is_a?(SystemExit) ? $!.status : SimpleCov::ExitCodes::EXCEPTION
else
# Store the exit status of the test run since it goes away
# after calling the at_exit proc...
SimpleCov::ExitCodes::SUCCESS
end

SimpleCov.at_exit.call

Expand Down
2 changes: 1 addition & 1 deletion lib/simplecov/jruby_fix.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
# This monkey patches Coverage to address those issues
module Coverage
class << self
alias_method :__broken_result__, :result
alias __broken_result__ result

def result # rubocop:disable Metrics/MethodLength
fixed = {}
Expand Down
2 changes: 1 addition & 1 deletion lib/simplecov/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class Result
attr_reader :original_result
# Returns all files that are applicable to this result (sans filters!) as instances of SimpleCov::SourceFile. Aliased as :source_files
attr_reader :files
alias_method :source_files, :files
alias source_files files
# Explicitly set the Time this result has been created
attr_writer :created_at
# Explicitly set the command name that was used for this coverage result. Defaults to SimpleCov.command_name
Expand Down
16 changes: 8 additions & 8 deletions lib/simplecov/source_file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ class Line
attr_reader :skipped

# Lets grab some fancy aliases, shall we?
alias_method :source, :src
alias_method :line, :line_number
alias_method :number, :line_number
alias source src
alias line line_number
alias number line_number

def initialize(src, line_number, coverage)
fail ArgumentError, "Only String accepted for source" unless src.is_a?(String)
Expand Down Expand Up @@ -76,7 +76,7 @@ def status
attr_reader :coverage
# The source code for this file. Aliased as :source
attr_reader :src
alias_method :source, :src
alias source src

def initialize(filename, coverage)
@filename = filename
Expand All @@ -102,7 +102,7 @@ def lines
process_skipped_lines!
@lines
end
alias_method :source_lines, :lines
alias source_lines lines

# Access SimpleCov::SourceFile::Line source lines by line number
def line(number)
Expand All @@ -116,7 +116,7 @@ def covered_percent
if relevant_lines.zero?
0.0
else
Float((covered_lines.count) * 100.0 / relevant_lines.to_f)
Float(covered_lines.count * 100.0 / relevant_lines.to_f)
end
end

Expand Down Expand Up @@ -172,8 +172,8 @@ def process_skipped_lines!
lines.each do |line|
if line.src =~ /^([\s]*)#([\s]*)(\:#{SimpleCov.nocov_token}\:)/
skipping = !skipping
else
line.skipped! if skipping
elsif skipping
line.skipped!
end
end
end
Expand Down
15 changes: 9 additions & 6 deletions lib/simplecov/version.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
module SimpleCov
VERSION = "0.11.1"
def VERSION.to_a
version = "0.11.1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not directly VERSION = '123'.freeze?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, you're adding methods to the object afterwards. Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a new cop that requires all literals assigned to constants to be frozen. Because the method definitions that follow modify the VERSION string, I had to do these gymnastics to make everything happy.

Perhaps we should create a Version module and make those methods class methods of that module?

I'm cool with going with whatever approach you all prefer!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, I can disable that cop.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, dammit - I had overlooked that we already were modifying the const object with those methods. You're right, let's keep it this way!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think freezing the version constant is actually breaking the simplecov-teamcity-summary with older versions of rubygems. The simplecov-teamcity-summary has the following code:
Gem::Version.new(SimpleCov::VERSION)
and rubygems in version 1.8.23 has @version.strip! in its constructor. In newer versions of rubygems they're using @version.strip so I don't think it's an issue there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following is also broken with that rubygems version:

$ git clone https://github.com/colszowka/simplecov.git
...
$ cd simplecov/
$ bundle
[!] There was an error parsing `Gemfile`: There was a RuntimeError while loading simplecov.gemspec: 
can't modify frozen String from
  ~/Workspace/simplecov/simplecov.gemspec:6:in `block in <main>'
. Bundler cannot continue.

 #  from ~/Workspace/simplecov/Gemfile:34
 #  -------------------------------------------
 #  
 >  gemspec
 #  -------------------------------------------
$ gem --version
1.8.23.2
$ ruby --version
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-darwin14.5.0]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, there are the most amazing bugs from the simplest things.

I'm somewhat torn on this one because (see #458) we basically shouldn't be spending time on supporting rubies that were EOLd a year ago but this is such a relatively small thing to fix and make things work on old rubies again. Maybe simplecov should even outright refuse to be compatible with outdated rubies to make people aware of the fact they're exposing themselves to unfixed security and performance problems.

Anyway, could you please send in a PR that disables the relevant Cop, with a comment about why it's disabled and a link to here as well as the unfreezing of the version string? Also, please upgrade your Ruby, in my experience the upgrades from 1.9 on are very easy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #461


def version.to_a
split(".").map(&:to_i)
end

def VERSION.major
def version.major
to_a[0]
end

def VERSION.minor
def version.minor
to_a[1]
end

def VERSION.patch
def version.patch
to_a[2]
end

def VERSION.pre
def version.pre
to_a[3]
end

VERSION = version.freeze
end
2 changes: 1 addition & 1 deletion spec/source_file_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
require "helper"

describe SimpleCov::SourceFile do
COVERAGE_FOR_SAMPLE_RB = [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil]
COVERAGE_FOR_SAMPLE_RB = [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil, nil, nil, nil, nil, nil, nil].freeze
context "a source file initialized with some coverage data" do
subject do
SimpleCov::SourceFile.new(source_fixture("sample.rb"), COVERAGE_FOR_SAMPLE_RB)
Expand Down