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

c4d0c8740d | the | 3398 fixed issues | 1737 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    def self.status_code(code)
 5      define_method(:status_code) { code }
 6      if match = BundlerError.all_errors.find {|_k, v| v == code }
 7        error, _ = match
 8        raise ArgumentError,
 9          "Trying to register #{self} for status code #{code} but #{error} is already registered"
12    end
13
14    if e = example.exception
15      new_exception = e.exception(e.message + "[Retried #{retries} times]")
16      new_exception.set_backtrace e.backtrace
17      example.instance_variable_set(:@exception, new_exception)
14      editor = [ENV["BUNDLER_EDITOR"], ENV["VISUAL"], ENV["EDITOR"]].find {|e| !e.nil? && !e.empty? }
15      return Bundler.ui.info("To open a bundled gem, set $EDITOR or $BUNDLER_EDITOR") unless editor
16      return unless spec = Bundler::CLI::Common.select_spec(name, :regex_match)
17      path = spec.full_gem_path
18      Dir.chdir(path) do
19        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?)
15      # (see Action#up)
16      def up(graph)
17        if existing = graph.vertices[name]
18          @existing_payload = existing.payload
19          @existing_root = existing.root
20        end
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
23      return true if Bundler.settings[:disable_checksum_validation]
24      return true unless checksum
25      return true unless source = @package.instance_variable_get(:@gem)
26      return true unless source.respond_to?(:with_read_io)
27      digest = source.with_read_io do |io|
28        digest = Digest::SHA256.new
23      validate_cmd!
24      SharedHelpers.set_bundle_environment
25      if bin_path = Bundler.which(cmd)
26        if !Bundler.settings[:disable_exec_load] && ruby_shebang?(bin_path)
27          return kernel_load(bin_path, *args)
28        end
23
24      loop do
25        break unless dep = deps.shift
26        next if !handled.add?(dep) || skip.include?(dep.name)
27
28        if spec = spec_for_dependency(dep, match_current_platform)
26        next if !handled.add?(dep) || skip.include?(dep.name)
27
28        if spec = spec_for_dependency(dep, match_current_platform)
29          specs << spec
30
31          spec.dependencies.each do |d|
33              t << "  " * depth << req.to_s
34              unless tree.last == req
35                if spec = conflict.activated_by_name[req.name]
36                  t << %( was resolved to #{spec.version}, which)
37                end
38                t << %( depends on)
34        else
35          Bundler.load_marshal Gem.inflate(downloader.fetch(uri).body)
36        end
37      rescue MarshalError
38        raise HTTPError, "Gemspec #{spec} contained invalid data.\n" \
39          "Your network or your gem server is probably having issues right now."
35      if args.empty?
36        if options[:parseable]
37          if value = Bundler.settings[name]
38            Bundler.ui.info("#{name}=#{value}")
39          end
40          return
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 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
37
38      # See Gem::Specification#add_self_to_load_path (since RubyGems 1.8)
39      if insert_index = Bundler.rubygems.load_path_insert_index
40        # Gem directories must come after -I and ENV['RUBYLIB']
41        $LOAD_PATH.insert(insert_index, *load_paths)
42      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
41      end
42
43      if spec = lookup["bundler"].first
44        specs << spec
45      end
46
54      last_empty_key = nil
55      str.split(/\r?\n/).each do |line|
56        if match = HASH_REGEX.match(line)
57          indent, key, _, val = match.captures
58          key = convert_to_backward_compatible_key(key)
59          depth = indent.scan(/  /).length
58
59      def delete_first(array, item)
60        return unless index = array.index(item)
61        array.delete_at(index)
62      end
63    end
59
60  def namespace(name)
61    if klass = Bundler::Thor::RakeCompat.rake_classes.last # rubocop:disable AssignmentInCondition
62      const_name = Bundler::Thor::Util.camel_case(name.to_s).to_sym
63      klass.const_set(const_name, Class.new(Bundler::Thor))
64      new_klass = klass.const_get(const_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


41    # @param value [Symbol] One of three Symbols: :major, :minor or :patch.
42    def level=(value)
43      v = case value
44          when String, Symbol
45            value.to_sym
46      end
52      default = value
53
54      type = case value
55      when Symbol
56        default = nil
57        if VALID_TYPES.include?(value)
54
55    def default_banner
56      case type
57      when :boolean
58        nil
59      when :string, :default
65        next false if curr_range.single? && neqs.include?(curr_range.left.version)
66        next curr_range if last_range.right.version == ReqR::INFINITY
67        case last_range.right.version <=> curr_range.left.version
68        when 1 then next curr_range
69        when 0 then next(last_range.right.inclusive && curr_range.left.inclusive && !neqs.include?(curr_range.left.version) && curr_range)
70        when -1 then next false
80
81    def not_local_engine_version
82      case not_local_tag
83      when :ruby
84        not_local_ruby_version
85      when :jruby
82        config[:test] = test_framework
83        config[:test_framework_version] = TEST_FRAMEWORK_VERSIONS[test_framework]
84
85        templates.merge!(".travis.yml.tt" => ".travis.yml")
86
87        case test_framework
82
83          if is_switch
84            case shifted
85            when SHORT_SQ_RE
86              unshift($1.split("").map { |f| "-#{f}" })
87              next
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
106      data.each do |k, v|
107        next unless v
108        case k.to_s
109        when "checksum"
110          @checksum = v.last
111        when "rubygems"
115
116    def validate_default_type!
117      default_type = case @default
118      when nil
119        return
120      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
306
307    def _cleanup_options_and_set(options, key) #:nodoc:
308      case options
309      when Array
310        %w(--force -f --skip -s).each { |i| options.delete(i) }
311        options << "--#{key}"
477        problem, expected, actual = diff
478
479        msg = case problem
480              when :engine
481                "Your Ruby engine is #{actual}, but your Gemfile specified #{expected}"
482              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
3begin
4  gem "psych"
5rescue LoadError
6end if defined?(gem)
7
8# Psych could be in the stdlib
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'
 5  require "openssl"
 6rescue LoadError
 7  # some Ruby builds don't have OpenSSL
 8end
 9module Bundler
10  module Persistent
11  require "psych" unless defined?(Syck)
12rescue LoadError
13  # Apparently Psych wasn't available. Oh well.
14end
15
16# At least load the YAML stdlib, whatever that may be
11begin
12  require 'net/http/pipeline'
13rescue LoadError
14end
15
16autoload :OpenSSL, 'openssl'
20      return uri.to_s if uri_to_anonymize.is_a?(String)
21    rescue URI::InvalidURIError # uri is not canonical uri scheme
22      uri
23    end
24
25    def credential_filtered_string(str_to_filter, uri)
124      @ruby_version ||= RubyVersion.new(ruby_version, patchlevel, ruby_engine, ruby_engine_version)
125    end
126
127    def to_gem_version_with_patchlevel
128      @gem_version_with_patch ||= begin
129        Gem::Version.create("#{@gem_version}.#{@patchlevel}")
219            Pathname.new(p).relative_path_from(gem_dir).to_s
220          rescue ArgumentError
221            p
222          end
223        end.compact
224
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
613          begin
614            value.dup
615          rescue TypeError
616            value
617          end
618
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


 9      Bundler.setup
10    rescue Bundler::BundlerError => e
11      puts "\e[31m#{e.message}\e[0m"
12      puts e.backtrace.join("\n") if ENV["DEBUG"]
13      if e.is_a?(Bundler::GemNotFound)
14        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
10    rescue Bundler::BundlerError => e
11      puts "\e[31m#{e.message}\e[0m"
12      puts e.backtrace.join("\n") if ENV["DEBUG"]
13      if e.is_a?(Bundler::GemNotFound)
14        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
15      end
12      puts e.backtrace.join("\n") if ENV["DEBUG"]
13      if e.is_a?(Bundler::GemNotFound)
14        puts "\e[33mRun `bundle install` to install missing gems.\e[0m"
15      end
16      exit e.status_code
17    end
22      requested_range_for(response_body)
23    rescue => e
24      puts e
25      puts e.backtrace
26      raise
27    end
23    rescue => e
24      puts e
25      puts e.backtrace
26      raise
27    end
28
30      end
31
32      puts "Writing new #{gemfile} to #{SharedHelpers.pwd}/#{gemfile}"
33    end
34
35  private
38    # @return [void]
39    def after_resolution
40      output.puts
41    end
42
43    # Conveys debug information to the user.
39        FileUtils.rm_rf(Path.base_system_gems)
40        FileUtils.mkdir_p(Path.base_system_gems)
41        puts "installing gems for the tests to use..."
42        install_gems(DEPS)
43        File.open(manifest_path, "w") {|f| f << manifest.join }
44      end
49        debug_info = yield
50        debug_info = debug_info.inspect unless debug_info.is_a?(String)
51        output.puts debug_info.split("\n").map { |s| ' ' * depth + s }
52      end
53    end
54
51
52      if print
53        puts definition.to_lock
54      else
55        file = options[:lockfile]
56        file = file ? File.expand_path(file) : Bundler.default_lockfile
55        file = options[:lockfile]
56        file = file ? File.expand_path(file) : Bundler.default_lockfile
57        puts "Writing lockfile to #{file}"
58        definition.lock(file)
59      end
60
57      deps = reqs.concat(no_reqs).join(" ")
58      cmd = "gem install #{deps} --no-rdoc --no-ri --conservative"
59      puts cmd
60      system(cmd) || raise("Installing gems #{deps} for the tests to use failed!")
61    end
62  end
85          Kernel.exec "man #{man_pages[command]}"
86        else
87          puts File.read("#{man_path}/#{File.basename(man_pages[command])}.txt")
88        end
89      elsif command_path = Bundler.which("bundler-#{cli}")
90        Kernel.exec(command_path, "--help")
123    rescue ArgumentError => e
124      raise unless e.message.include?(ILLFORMED_MESSAGE)
125      puts # we shouldn't print the error message on the "fetching info" status line
126      raise GemspecError,
127        "Unfortunately, the gem #{name} (#{version}) has an invalid " \
128        "gemspec.\nPlease ask the gem author to yank the bad version to fix " \
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"
142          # Don't output trailing spaces when printing the last column
143          if ((((index + 1) % (terminal_width / colwidth))).zero? && !index.zero?) || index + 1 == array.length
144            stdout.puts value
145          else
146            stdout.printf("%-#{colwidth}s", value)
147          end
207
208          sentence = truncate(sentence, options[:truncate]) if options[:truncate]
209          stdout.puts sentence
210        end
211      end
212
231        paras.each do |para|
232          para.split("\n").each do |line|
233            stdout.puts line.insert(0, " " * indent)
234          end
235          stdout.puts unless para == paras.last
236        end
233            stdout.puts line.insert(0, " " * indent)
234          end
235          stdout.puts unless para == paras.last
236        end
237      end
238
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...