Just so you know …

Your are not authenticated, you have a restricted access. Feel free to login to get a better user experience.
Free for open source projects. Sign up here

Review of branch master

db9c056a2a | the | 151 fixed issues | 37 to go.   |   View branch on GitHub    |   Badge urls

This is a review of an integration branch. Instead of showing the difference between the given branch and the integration, we're showing here below all problems we found in the current codebase, so the list may be long.

This is a public review of a branch. It means that you can read it, even if you never contributed code to it. Be aware that we may hide some sensitive information regarding security issues.

12          strategy, *strategy_opts = args
13          strategy_type =
14            case strategy
15            when Symbol
16              map[strategy] || raise(InvalidStrategy, "The strategy '#{strategy}' is not understood. Please use one of #{map.keys.join(',')}")
17            when Class

Why

This check reports case statements that don't have an else clause that would be executed when no cases match. If the tested value will never adopt any other values than the ones tested for in the cases, this should be expressed in the code by throwing an exception in the else clause.

Check that case statements have an else statement so that all cases are covered.

case foo
  when 'bar': 'ok'
  when 'foo': 'bad'
end

What happens when foo is equal to 'biz'?

How to fix

Add an else clause and throw an exception

case foo
when 'bar':
    'ok'
when 'foo':
    'bad'
else
    raise "unsupported value : '#{foo}'"
end

Or define a default case

case foo
when 'bar':
    'ok'
when 'foo':
    'not too bad'
else
    'really bad'
end

More info

How Ruby's switch works behind the hood.

Know a good reference on this subject ? Tell us about it!

Occured at


2  # Try to load it so we can assign @_result below if needed.
3  require 'test/unit/testresult'
4rescue LoadError => ignore
5end
6
7module Cucumber #:nodoc:
10  end
11
12rescue LoadError => ignore_if_database_cleaner_not_present
13end

Why

Looking for a bug where an exception is caught blindly takes hours to fix.

begin
  call_method
rescue
  # something really bad happened, but let's continue like everything is fine
end

How to fix

Take a few seconds to log or re-raise an enriched version of the exception.

begin
  call_method
rescue => e
  logger.error "#{e.message} \n #{e.backtrace.join("\n ")}"
  raise e
end

More info

Discussion about empty catch clause - It is about Java, but the principle and problems are exactly the same as those related to empty rescue in Ruby.

Know a good reference on this subject ? Tell us about it!

45
46        # Since gsub_file doesn't ask the user, just inform user that the file was overwritten.
47        puts '       force  config/database.yml'
48      end
49    end
50

Why

You don't want to clutter your logs with raw puts, pp, or p. Output using p will not always appear in your logs, nor will it inherit from any log config you may have (to add information such as the timestamp).

How to fix

In a Rails application

Use Rails.logger instead.

In Ruby code

Just use the Logger class.

In unit and integration tests

This is often a sign that you are missing some asserts and other checks.

More info

  • Rails logger
  • Stdlib logger
  • Better logs for Rails using lograge .
  • Fully configurable logs for any Ruby application using log4r (look what Mitchell Hasimoto of Vagrant fame says about it ).

Know a good reference on this subject ? Tell us about it!

Loading more violations...