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

5f9a30cf4b | the | 3506 fixed issues | 1919 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.

Info

For performance reasons, you only see the 20 first locations.

Occured at


 4
 5module Gem
 6  if version = ENV["BUNDLER_SPEC_RUBYGEMS_VERSION"]
 7    remove_const(:VERSION) if const_defined?(:VERSION)
 8    VERSION = version
 9  end
 5    def self.status_code(code)
 6      define_method(:status_code) { code }
 7      if match = BundlerError.all_errors.find {|_k, v| v == code }
 8        error, _ = match
 9        raise ArgumentError,
10          "Trying to register #{self} for status code #{code} but #{error} is already registered"
10
11      def initialize(all_specs)
12        raise ArgumentError, "cannot initialize with an empty value" unless exemplary_spec = all_specs.first
13        @name = exemplary_spec.name
14        @version = exemplary_spec.version
15        @source = exemplary_spec.source
13    end
14
15    if e = example.exception
16      new_exception = e.exception(e.message + "[Retried #{retries} times]")
17      new_exception.set_backtrace e.backtrace
18      example.instance_variable_set(:@exception, new_exception)
15      editor = [ENV["BUNDLER_EDITOR"], ENV["VISUAL"], ENV["EDITOR"]].find {|e| !e.nil? && !e.empty? }
16      return Bundler.ui.info("To open a bundled gem, set $EDITOR or $BUNDLER_EDITOR") unless editor
17      return unless spec = Bundler::CLI::Common.select_spec(name, :regex_match)
18      path = spec.full_gem_path
19      Dir.chdir(path) do
20        command = Shellwords.split(editor) + [path]
15          ::Readline.completion_append_character = nil
16          # Ruby 1.8.7 does not allow Readline.completion_proc= to receive nil.
17          if complete = completion_proc
18            ::Readline.completion_proc = complete
19          end
20          ::Readline.readline(prompt, add_to_history?)
16      return
17    end
18    return unless bundler_spec = Gem.loaded_specs["bundler"]
19    return if bundler_spec.version == VERSION
20    bundler_spec.version = Bundler::VERSION
21  end
16      # (see Action#up)
17      def up(graph)
18        if existing = graph.vertices[name]
19          @existing_payload = existing.payload
20          @existing_root = existing.root
21        end
20            s.loaded_from = File.expand_path("..", __FILE__)
21          end
22          if loaded_spec = nil && Bundler.rubygems.loaded_specs("bundler")
23            idx << loaded_spec # this has to come after the fake gemspec, to override it
24          elsif local_spec = Bundler.rubygems.find_name("bundler").find {|s| s.version.to_s == VERSION }
25            idx << local_spec
21    def build_extensions
22      extension_cache_path = options[:bundler_extension_cache_path]
23      return super unless extension_cache_path && extension_dir = Bundler.rubygems.spec_extension_dir(spec)
24
25      extension_dir = Pathname.new(extension_dir)
26      build_complete = SharedHelpers.filesystem_access(extension_cache_path.join("gem.build_complete"), :read, &:file?)
24          elsif local_spec = Bundler.rubygems.find_name("bundler").find {|s| s.version.to_s == VERSION }
25            idx << local_spec
26          end
27
28          idx.each {|s| s.source = self }
29        end
24
25      loop do
26        break unless dep = deps.shift
27        next if !handled.add?(dep) || skip.include?(dep.name)
28
29        if spec = spec_for_dependency(dep, match_current_platform)
24      validate_cmd!
25      SharedHelpers.set_bundle_environment
26      if bin_path = Bundler.which(cmd)
27        if !Bundler.settings[:disable_exec_load] && ruby_shebang?(bin_path)
28          return kernel_load(bin_path, *args)
29        end
26      def to_specs
27        @activated_platforms.map do |p|
28          next unless s = @specs[p]
29          lazy_spec = LazySpecification.new(name, version, s.platform, source)
30          lazy_spec.dependencies.replace s.dependencies
31          lazy_spec
27        next if !handled.add?(dep) || skip.include?(dep.name)
28
29        if spec = spec_for_dependency(dep, match_current_platform)
30          specs << spec
31
32          spec.dependencies.each do |d|
35        else
36          Bundler.load_marshal Gem.inflate(downloader.fetch(uri).body)
37        end
38      rescue MarshalError
39        raise HTTPError, "Gemspec #{spec} contained invalid data.\n" \
40          "Your network or your gem server is probably having issues right now."
36        if iv_list.include?(ivname) and
37           value = instance_variable_get(ivname)
38          ssl_parameters[name] = value
39        end
40      end
41      unless @ssl_context then
36
37    HASH_REGEX = /
38      ^
39      ([ ]*) # indentations
40      (.+) # key
41      (?::(?=(?:\s|$))) # :  (without the lookahead the #key includes this when : is present in value)
42
36      if args.empty?
37        if options[:parseable]
38          if value = Bundler.settings[name]
39            Bundler.ui.info("#{name}=#{value}")
40          end
41          return
38
39      # See Gem::Specification#add_self_to_load_path (since RubyGems 1.8)
40      if insert_index = Bundler.rubygems.load_path_insert_index
41        # Gem directories must come after -I and ENV['RUBYLIB']
42        $LOAD_PATH.insert(insert_index, *load_paths)
43      else
41    task = super
42
43    if klass = Bundler::Thor::RakeCompat.rake_classes.last # rubocop:disable AssignmentInCondition
44      non_namespaced_name = task.name.split(":").last
45
46      description = non_namespaced_name

Why

It was reported that an assignment was used in a conditional as in

something(var) if var = method() #bad

Using assignment in a conditional could lead to dangerous mistakes mainly because they would be cofounded with an equality ==.

Applies to

  • if
  • else
  • while
  • until

How to fix

It's often a typo.

Did you mean an equality == instead of an assignment =?

something(var) if var == method()

Sometimes it's not a typo.

Don't use the return value of = (an assignment) in conditional expressions.

# bad (+ a warning)
if (v = array.grep(/foo/))
  do_something(v)
  # ...
end

# bad (+ a warning)
if v = array.grep(/foo/)
  do_something(v)
  # ...
end

but extract the assignment

# good
v = array.grep(/foo/)
if v
  do_something(v)
  # ...
end

More info

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

Occured at


42    # @param value [Symbol] One of three Symbols: :major, :minor or :patch.
43    def level=(value)
44      v = case value
45          when String, Symbol
46            value.to_sym
47      end
53      default = value
54
55      type = case value
56      when Symbol
57        default = nil
58        if VALID_TYPES.include?(value)
54
55    def default_banner
56      case type
57      when :boolean
58        nil
59      when :string, :default
66        next false if curr_range.single? && neqs.include?(curr_range.left.version)
67        next curr_range if last_range.right.version == ReqR::INFINITY
68        case last_range.right.version <=> curr_range.left.version
69        when 1 then next curr_range
70        when 0 then next(last_range.right.inclusive && curr_range.left.inclusive && !neqs.include?(curr_range.left.version) && curr_range)
71        when -1 then next false
83
84          if is_switch
85            case shifted
86            when SHORT_SQ_RE
87              unshift($1.split("").map { |f| "-#{f}" })
88              next
83        config[:test] = test_framework
84        config[:test_framework_version] = TEST_FRAMEWORK_VERSIONS[test_framework]
85
86        templates.merge!(".travis.yml.tt" => ".travis.yml")
87
88        case test_framework
85
86    def not_local_engine_version
87      case not_local_tag
88      when :ruby
89        not_local_ruby_version
90      when :jruby
104      prerelease_text = bundler_version.prerelease? ? " --pre" : ""
105      current_version = Gem::Version.create(Bundler::VERSION)
106      case current_version.segments.first <=> bundler_version.segments.first
107      when -1
108        raise LockfileError, "You must use Bundler #{bundler_version.segments.first} or greater with this lockfile."
109      when 0
115      data.each do |k, v|
116        next unless v
117        case k.to_s
118        when "checksum"
119          @checksum = v.last
120        when "rubygems"
116
117    def validate_default_type!
118      default_type = case @default
119      when nil
120        return
121      when TrueClass, FalseClass
129      case line
130      when SPECS
131        case @type
132        when PATH
133          @current_source = TYPES[@type].from_lock(@opts)
134          @sources << @current_source
266    with_padding do
267      if block
268        case block.arity
269        when 3
270          yield(self, klass, command)
271        when 2
309
310    def _cleanup_options_and_set(options, key) #:nodoc:
311      case options
312      when Array
313        %w(--force -f --skip -s).each { |i| options.delete(i) }
314        options << "--#{key}"
462        problem, expected, actual = diff
463
464        msg = case problem
465              when :engine
466                "Your Ruby engine is #{actual}, but your Gemfile specified #{expected}"
467              when :version
746
747  def idempotent? req
748    case req
749    when Net::HTTP::Delete, Net::HTTP::Get, Net::HTTP::Head,
750         Net::HTTP::Options, Net::HTTP::Put, Net::HTTP::Trace then
751      true

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


1begin
2  require "readline"
3rescue LoadError
4end
5
6class Bundler::Thor
3  require 'net/https'
4rescue LoadError
5  # net/https or openssl
6end if RUBY_VERSION < '1.9' # but only for 1.8
7require 'bundler/vendor/net-http-persistent/lib/net/http/faster'
8require 'uri'
 4begin
 5  gem "psych"
 6rescue LoadError
 7end if defined?(gem)
 8
 9# Psych could be in the stdlib
 6  require "openssl"
 7rescue LoadError
 8  # some Ruby builds don't have OpenSSL
 9end
10module Bundler
11  module Persistent
11begin
12  require 'net/http/pipeline'
13rescue LoadError
14end
15
16autoload :OpenSSL, 'openssl'
12  require "psych" unless defined?(Syck)
13rescue LoadError
14  # Apparently Psych wasn't available. Oh well.
15end
16
17# At least load the YAML stdlib, whatever that may be
17      # create the lock file or is using NFS where
18      # locks are not available we skip locking.
19      yield
20    ensure
21      FileUtils.rm_f(lock_file_path) if has_lock
22    end
21      return uri.to_s if uri_to_anonymize.is_a?(String)
22    rescue URI::InvalidURIError # uri is not canonical uri scheme
23      uri
24    end
25
26    def credential_filtered_string(str_to_filter, uri)
125      @ruby_version ||= RubyVersion.new(ruby_version, patchlevel, ruby_engine, ruby_engine_version)
126    end
127
128    def to_gem_version_with_patchlevel
129      @gem_version_with_patch ||= begin
130        Gem::Version.create("#{@gem_version}.#{@patchlevel}")
220            Pathname.new(p).relative_path_from(gem_dir).to_s
221          rescue ArgumentError
222            p
223          end
224        end.compact
225
238              end
239            rescue NotImplementedError # rubocop:disable HandleExceptions
240              # just ignore on windows
241            end
242          end
243
270    end
271  rescue
272    # ignore StandardErrors, we've probably found the idle timeout.
273  ensure
274    http.shutdown
275
636          begin
637            value.dup
638          rescue TypeError
639            value
640          end
641
720
721    connection.finish
722  rescue IOError
723  end
724
725  def http_class # :nodoc:

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!

Info

For performance reasons, you only see the 20 first locations.

Occured at


10      Bundler.setup
11    rescue Bundler::BundlerError => e
12      puts "\e[31m#{e.message}\e[0m"
13      puts e.backtrace.join("\n") if ENV["DEBUG"]
14      if e.is_a?(Bundler::GemNotFound)
15        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
11    rescue Bundler::BundlerError => e
12      puts "\e[31m#{e.message}\e[0m"
13      puts e.backtrace.join("\n") if ENV["DEBUG"]
14      if e.is_a?(Bundler::GemNotFound)
15        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
16      end
13      puts e.backtrace.join("\n") if ENV["DEBUG"]
14      if e.is_a?(Bundler::GemNotFound)
15        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
16      end
17      exit e.status_code
18    end
23      requested_range_for(response_body)
24    rescue => e
25      puts e
26      puts e.backtrace
27      raise
28    end
24    rescue => e
25      puts e
26      puts e.backtrace
27      raise
28    end
29
31      end
32
33      puts "Writing new #{gemfile} to #{SharedHelpers.pwd}/#{gemfile}"
34    end
35
36  private
39    # @return [void]
40    def after_resolution
41      output.puts
42    end
43
44    # Conveys debug information to the user.
43        FileUtils.rm_rf(Path.base_system_gems)
44        FileUtils.mkdir_p(Path.base_system_gems)
45        puts "installing gems for the tests to use..."
46        install_gems(DEPS)
47        File.open(manifest_path, "w") {|f| f << manifest.join }
48      end
51        debug_info = debug_info.inspect unless debug_info.is_a?(String)
52        debug_info = debug_info.split("\n").map { |s| ":#{depth.to_s.rjust 4}: #{s}" }
53        output.puts debug_info
54      end
55    end
56
52
53      if print
54        puts definition.to_lock
55      else
56        file = options[:lockfile]
57        file = file ? File.expand_path(file) : Bundler.default_lockfile
56        file = options[:lockfile]
57        file = file ? File.expand_path(file) : Bundler.default_lockfile
58        puts "Writing lockfile to #{file}"
59        definition.lock(file)
60      end
61
61      deps = reqs.concat(no_reqs).join(" ")
62      cmd = "gem install #{deps} --no-rdoc --no-ri --conservative"
63      puts cmd
64      system(cmd) || raise("Installing gems #{deps} for the tests to use failed!")
65    end
66  end
93      if man_pages.include?(command)
94        if Bundler.which("man") && man_path !~ %r{^file:/.+!/META-INF/jruby.home/.+}
95          Kernel.exec "man #{man_pages[command]}"
96        else
97          puts File.read("#{man_path}/#{File.basename(man_pages[command])}.txt")
98        end
126    save_yaml(thor_yaml)
127
128    puts "Done."
129  end
130
131  desc "update NAME", "Update a Bundler::Thor file from its original location"
132    rescue ArgumentError => e
133      raise unless e.message.include?(ILLFORMED_MESSAGE)
134      puts # we shouldn't print the error message on the "fetching info" status line
135      raise GemspecError,
136        "Unfortunately, the gem #{name} (#{version}) has an invalid " \
137        "gemspec.\nPlease ask the gem author to yank the bad version to fix " \
139          # Don't output trailing spaces when printing the last column
140          if ((((index + 1) % (terminal_width / colwidth))).zero? && !index.zero?) || index + 1 == array.length
141            stdout.puts value
142          else
143            stdout.printf("%-#{colwidth}s", value)
144          end
204
205          sentence = truncate(sentence, options[:truncate]) if options[:truncate]
206          stdout.puts sentence
207        end
208      end
209
228        paras.each do |para|
229          para.split("\n").each do |line|
230            stdout.puts line.insert(0, " " * indent)
231          end
232          stdout.puts unless para == paras.last
233        end
230            stdout.puts line.insert(0, " " * indent)
231          end
232          stdout.puts unless para == paras.last
233        end
234      end
235
282      it "is passed to the hook" do
283        out = capture(:stdout) do
284          Plugin.hook("event-1") { puts "win" }
285        end.strip
286
287        expect(out).to eq("win")
293      #
294      def error(statement)
295        stderr.puts statement
296      end
297
298      # Apply color to the given string with optional bold. Disabled in the

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...