Skip to content

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also .

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also .
...
  • 18 commits
  • 18 files changed
  • 0 commit comments
  • 3 contributors
Commits on Sep 24, 2012
@flyerhzm flyerhzm ProtectMassAssignmentChecker supports strong_parameters gem now 880d1de
Commits on Oct 02, 2012
@flyerhzm flyerhzm use && instead of ; for multiple shell commands 0e8586f
Commits on Oct 06, 2012
@justindell justindell fix bug when creating html with no errors present 373b48d
Commits on Oct 07, 2012
@flyerhzm flyerhzm Merge pull request #134 from justindell/master
Fix bug when creating html with no errors present
798cc10
Commits on Oct 09, 2012
@flyerhzm flyerhzm don't remove empty application_helper.rb 31336c9
@flyerhzm flyerhzm remove errors_filter_block a12cae7
@flyerhzm flyerhzm add yaml output 659f504
@flyerhzm flyerhzm analyze_vcs for yaml format as well 3ef8af0
@flyerhzm flyerhzm remove Error#highlight e7fb088
Commits on Oct 24, 2012
@RichardBradley RichardBradley Add support for t.index style indices in schema.rb, as added by e.g. …
…schema_plus
053dc91
Commits on Oct 25, 2012
@flyerhzm flyerhzm Merge pull request #135 from RichardBradley/master
fix index required false-positive
e8db874
Commits on Oct 29, 2012
@RichardBradley RichardBradley Disallow catching 'Exception' -- a common mistake by Java or C# devs …
…in ruby
0405f68
Commits on Nov 02, 2012
@flyerhzm flyerhzm Merge pull request #137 from RichardBradley/not_rescue_exception
Disallow catching 'Exception'
de1521a
@flyerhzm flyerhzm change url for NotRescueExceptionReview 8b495d5
@flyerhzm flyerhzm should not set execute permission for not_rescue_exception_review 917b960
@flyerhzm flyerhzm refactor not_rescue_exception_review to use exception_classes api of …
…rescue node
0c63447
@flyerhzm flyerhzm say we added "NotRescueExceptionCheck" in README 4a21062
@flyerhzm flyerhzm Bumping version to 1.12.0 068de79
View
2 .gitignore
@@ -7,4 +7,4 @@ pkg/**
rdoc/**
doc/**
.yardoc/**
-rails_best_practices_output.html
+rails_best_practices_output.*
View
4 README.md
@@ -128,6 +128,7 @@ Now you can customize this configuration file, the default configuration is as f
MoveFinderToNamedScopeCheck: { }
MoveModelLogicIntoModelCheck: { use_count: 4 }
NeedlessDeepNestingCheck: { nested_count: 2 }
+ NotRescueExceptionCheck: { }
NotUseDefaultRouteCheck: { }
NotUseTimeAgoInWordsCheck: {}
OveruseRouteCustomizationsCheck: { customize_count: 3 }
@@ -219,9 +220,10 @@ Other
1. Remove trailing whitespace
2. Remove tab (disabled by default)
-3. Hash Syntax (disabled by default)
+3. Hash syntax (disabled by default)
4. Use parentheses in method def (disabled by default)
5. Long line (disabled by default)
+6. Not rescue exception
## Write Your Own Check List
View
28 assets/result.html.erb
@@ -72,22 +72,24 @@
<% end %>
</p>
<table>
- <% columnize(@error_types, 3).each do |row| %>
+ <% unless @errors.empty? %>
+ <% columnize(@error_types, 3).each do |row| %>
+ <tr>
+ <% row.map { |error_type| error_type.split(':').last }.each do |error_type| %>
+ <td>
+ <input type="checkbox" class="error-type" id="<%= error_type %>" value="<%= error_type %>"
+ />&nbsp;<label for="<%= error_type %>"><%= error_type.sub(/(Check|Review)$/, '').gsub(/([a-z\d])([A-Z])/,'\1 \2') %></label>
+ </td>
+ <% end %>
+ </tr>
+ <% end %>
<tr>
- <% row.map { |error_type| error_type.split(':').last }.each do |error_type| %>
- <td>
- <input type="checkbox" class="error-type" id="<%= error_type %>" value="<%= error_type %>"
- />&nbsp;<label for="<%= error_type %>"><%= error_type.sub(/(Check|Review)$/, '').gsub(/([a-z\d])([A-Z])/,'\1 \2') %></label>
- </td>
- <% end %>
+ <td colspan="3">
+ <button id="show-all">Check all</button>
+ <button id="show-none">Uncheck all</button>
+ </td>
</tr>
<% end %>
- <tr>
- <td colspan="3">
- <button id="show-all">Check all</button>
- <button id="show-none">Uncheck all</button>
- </td>
- </tr>
</table>
<table class="result">
<thead>
View
28 lib/rails_best_practices/analyzer.rb
@@ -17,7 +17,7 @@ module RailsBestPractices
#
# After analyzing, output the violations.
class Analyzer
- attr_accessor :runner, :errors_filter_block
+ attr_accessor :runner
DEFAULT_CONFIG = File.join(File.dirname(__FILE__), "..", "..", "rails_best_practices.yml")
@@ -49,21 +49,22 @@ def generate
def analyze
@options["exclude"] ||= []
@options["only"] ||= []
- @options["output-file"] ||= "rails_best_practices_output.html"
Core::Runner.base_path = @path
@runner = Core::Runner.new
analyze_source_codes
analyze_vcs
-
- errors_filter_block.call(errors) if errors_filter_block
end
# Output the analyze result.
def output
if @options["format"] == 'html'
+ @options["output-file"] ||= "rails_best_practices_output.html"
output_html_errors
+ elsif @options["format"] == 'yaml'
+ @options["output-file"] ||= "rails_best_practices_output.yaml"
+ output_yaml_errors
else
output_terminal_errors
end
@@ -181,7 +182,7 @@ def output_terminal_errors
def load_hg_info
hg_progressbar = ProgressBar.new('Hg Info', errors.size) if display_bar?
errors.each do |error|
- hg_info = `cd #{@runner.class.base_path}; hg blame -lvcu #{error.filename[@runner.class.base_path.size..-1].gsub(/^\//, "")} | sed -n /:#{error.line_number.split(',').first}:/p`
+ hg_info = `cd #{@runner.class.base_path} && hg blame -lvcu #{error.filename[@runner.class.base_path.size..-1].gsub(/^\//, "")} | sed -n /:#{error.line_number.split(',').first}:/p`
unless hg_info == ""
hg_commit_username = hg_info.split(':')[0].strip
error.hg_username = hg_commit_username.split(/\ /)[0..-2].join(' ')
@@ -197,7 +198,7 @@ def load_git_info
git_progressbar = ProgressBar.new('Git Info', errors.size) if display_bar?
start = @runner.class.base_path =~ /\/$/ ? @runner.class.base_path.size : @runner.class.base_path.size + 1
errors.each do |error|
- git_info = `cd #{@runner.class.base_path}; git blame -L #{error.line_number.split(',').first},+1 #{error.filename[start..-1]}`
+ git_info = `cd #{@runner.class.base_path} && git blame -L #{error.line_number.split(',').first},+1 #{error.filename[start..-1]}`
unless git_info == ""
git_commit, git_username = git_info.split(/\d{4}-\d{2}-\d{2}/).first.split("(")
error.git_commit = git_commit.split(" ").first.strip
@@ -214,7 +215,7 @@ def output_html_errors
template = @options["template"] ? File.read(File.expand_path(@options["template"])) : File.read(File.join(File.dirname(__FILE__), "..", "..", "assets", "result.html.erb"))
if @options["with-github"]
- last_commit_id = @options["last-commit-id"] ? @options["last-commit-id"] : `cd #{@runner.class.base_path}; git rev-parse HEAD`.chomp
+ last_commit_id = @options["last-commit-id"] ? @options["last-commit-id"] : `cd #{@runner.class.base_path} && git rev-parse HEAD`.chomp
end
File.open(@options["output-file"], "w+") do |file|
eruby = Erubis::Eruby.new(template)
@@ -232,6 +233,13 @@ def output_html_errors
end
end
+ # output errors with yaml format.
+ def output_yaml_errors
+ File.open(@options["output-file"], "w+") do |file|
+ file.write YAML.dump(errors)
+ end
+ end
+
# plain output with color.
#
# @param [String] message to output
@@ -253,10 +261,8 @@ def analyze_source_codes
# analyze version control system info.
def analyze_vcs
- if @options["format"] == 'html'
- load_git_info if @options["with-git"]
- load_hg_info if @options["with-hg"]
- end
+ load_git_info if @options["with-git"]
+ load_hg_info if @options["with-hg"]
end
# if disaply progress bar.
View
3 lib/rails_best_practices/core/error.rb
@@ -6,7 +6,7 @@ module Core
# it indicates the filenname, line number and error message for the violation.
class Error < CodeAnalyzer::Warning
attr_reader :type, :url
- attr_accessor :git_commit, :git_username, :hg_commit, :hg_username, :highlight
+ attr_accessor :git_commit, :git_username, :hg_commit, :hg_username
def initialize(options={})
super
@@ -16,7 +16,6 @@ def initialize(options={})
@git_username = options[:git_username]
@hg_commit = options[:hg_commit]
@hg_username = options[:hg_username]
- @highlight = false
end
def short_filename
View
1 lib/rails_best_practices/reviews.rb
@@ -33,3 +33,4 @@
require 'rails_best_practices/reviews/protect_mass_assignment_review'
require 'rails_best_practices/reviews/use_parentheses_in_method_def_review'
require 'rails_best_practices/reviews/hash_syntax_review'
+require 'rails_best_practices/reviews/not_rescue_exception_review'
View
21 lib/rails_best_practices/reviews/always_add_db_index_review.rb
@@ -42,6 +42,8 @@ def initialize
add_callback :start_command_call do |node|
if %w(integer string).include? node.message.to_s
remember_foreign_key_columns(node)
+ elsif "index" == node.message.to_s
+ remember_index_columns_inside_table(node)
end
end
@@ -57,7 +59,7 @@ def initialize
when "create_table"
remember_table_nodes(node)
when "add_index"
- remember_index_columns(node)
+ remember_index_columns_outside_table(node)
end
end
@@ -81,15 +83,28 @@ def initialize
end
private
- # remember the node as index columns
- def remember_index_columns(node)
+ # remember the node as index columns, when used outside a table
+ # block, i.e.
+ # add_index :table_name, :column_name
+ def remember_index_columns_outside_table(node)
table_name = node.arguments.all.first.to_s
index_column = node.arguments.all[1].to_object
@index_columns[table_name] ||= []
@index_columns[table_name] << index_column
end
+ # remember the node as index columns, when used inside a table
+ # block, i.e.
+ # t.index [:column_name, ...]
+ def remember_index_columns_inside_table(node)
+ table_name = @table_name
+ index_column = node.arguments.all.first.to_object
+
+ @index_columns[table_name] ||= []
+ @index_columns[table_name] << index_column
+ end
+
# remember table nodes
def remember_table_nodes(node)
@table_name = node.arguments.all.first.to_s
View
28 lib/rails_best_practices/reviews/not_rescue_exception_review.rb
@@ -0,0 +1,28 @@
+# encoding: utf-8
+require 'rails_best_practices/reviews/review'
+
+module RailsBestPractices
+ module Reviews
+ # Review all code to make sure we don't rescue Exception
+ # This is a common mistake by Java or C# devs in ruby.
+ #
+ # See the best practice details here http://rails-bestpractices.com/posts/702-don-t-rescue-exception-rescue-standarderror
+ #
+ # Implementation:
+ #
+ # Review process:
+ # check all rescue node to see if its type is Exception
+ class NotRescueExceptionReview < Review
+ interesting_nodes :rescue
+ interesting_files ALL_FILES
+ url "http://rails-bestpractices.com/posts/702-don-t-rescue-exception-rescue-standarderror"
+
+ # check rescue node to see if its type is Exception
+ add_callback :start_rescue do |rescue_node|
+ if rescue_node.exception_classes.any? { |rescue_class| "Exception" == rescue_class.to_s }
+ add_error "not rescue Exception", rescue_node.file, rescue_node.exception_classes.first.line
+ end
+ end
+ end
+ end
+end
View
85 lib/rails_best_practices/reviews/protect_mass_assignment_review.rb
@@ -3,56 +3,89 @@
module RailsBestPractices
module Reviews
- # Review model files to make sure to use attr_accessible or attr_protected to protect mass assignment.
+ # Review model files to make sure to use attr_accessible, attr_protected or strong_parameters to protect mass assignment.
#
# See the best practices details here http://rails-bestpractices.com/posts/148-protect-mass-assignment.
#
# Implmentation:
#
# Review process:
- # check class node to see if there is a command with message attr_accessible or attr_protected.
+ # check nodes to see if there is a command with message attr_accessible or attr_protected,
+ # or include ActiveModel::ForbiddenAttributesProtection.
class ProtectMassAssignmentReview < Review
- interesting_nodes :class
interesting_files MODEL_FILES
+ interesting_nodes :class, :command, :var_ref, :vcall, :fcall
url "http://rails-bestpractices.com/posts/148-protect-mass-assignment"
- # check class node, grep all command nodes,
- # if config.active_record.whitelist_attributes is not set true,
- # and if none of them is with message attr_accessible or attr_protected,
- # and if not use devise or authlogic,
- # then it should add attr_accessible or attr_protected to protect mass assignment.
+ # we treat it as mass assignment by default.
add_callback :start_class do |node|
- if !whitelist_attributes_config? && !rails_builtin?(node) && !devise?(node) &&
- !authlogic?(node) && is_active_record?(node)
- add_error "protect mass assignment"
- end
+ @mass_assignement = true
+ end
+
+ # check if it is ActiveRecord::Base subclass and
+ # if it sets config.active_record.whitelist_attributes to true.
+ add_callback :end_class do |node|
+ check_active_record(node)
+ check_whitelist_attributes_config
+
+ add_error "protect mass assignment" if @mass_assignement
+ end
+
+ # check if it is attr_accessible or attr_protected command,
+ # if it uses strong_parameters,
+ # and if it uses devise.
+ add_callback :start_command do |node|
+ check_rails_builtin(node)
+ check_strong_parameters(node)
+ check_devise(node)
+ end
+
+ # check if it is attr_accessible, attr_protected, acts_as_authlogic without parameters.
+ add_callback :start_var_ref, :start_vcall do |node|
+ check_rails_builtin(node)
+ check_authlogic(node)
+ end
+
+ # check if it uses authlogic.
+ add_callback :start_fcall do |node|
+ check_authlogic(node)
end
private
- def whitelist_attributes_config?
- Prepares.configs["config.active_record.whitelist_attributes"] == "true"
+ def check_whitelist_attributes_config
+ if "true" == Prepares.configs["config.active_record.whitelist_attributes"]
+ @mass_assignement = false
+ end
end
- def rails_builtin?(node)
- node.grep_node(sexp_type: [:vcall, :var_ref], to_s: "attr_accessible").present? ||
- node.grep_node(sexp_type: :command, message: %w(attr_accessible attr_protected)).present?
+ def check_rails_builtin(node)
+ if [node.to_s, node.message.to_s].any? { |str| %w(attr_accessible attr_protected).include? str }
+ @mass_assignement = false
+ end
end
- def devise?(node)
- node.grep_node(sexp_type: :command, message: "devise").present?
+ def check_strong_parameters(command_node)
+ if "include" == command_node.message.to_s && "ActiveModel::ForbiddenAttributesProtection" == command_node.arguments.all.first.to_s
+ @mass_assignement = false
+ end
end
- def authlogic?(node)
- node.grep_node(sexp_type: [:vcall, :var_ref], to_s: "acts_as_authentic").present? ||
- node.grep_node(sexp_type: :fcall, message: "acts_as_authentic").present?
+ def check_devise(command_node)
+ if "devise" == command_node.message.to_s
+ @mass_assignement = false
+ end
end
- def is_active_record?(node)
- node.grep_node(sexp_type: [:const_path_ref, :@const], to_s: "ActiveRecord::Base").present?
+ def check_authlogic(node)
+ if [node.to_s, node.message.to_s].include? "acts_as_authentic"
+ @mass_assignement = false
+ end
end
- def is_active_record?(node)
- node.grep_node(:sexp_type => [:const_path_ref, :@const], :to_s => "ActiveRecord::Base").present?
+ def check_active_record(const_path_ref_node)
+ if "ActiveRecord::Base" != const_path_ref_node.base_class.to_s
+ @mass_assignement = false
+ end
end
end
end
View
11 lib/rails_best_practices/reviews/remove_empty_helpers_review.rb
@@ -17,11 +17,16 @@ class RemoveEmptyHelpersReview < Review
url "http://rails-bestpractices.com/posts/72-remove-empty-helpers"
# check the body of module node, if it is nil, then it should be removed.
- add_callback :start_module do |node|
- if s(:bodystmt, s(:stmts_add, s(:stmts_new), s(:void_stmt)), nil, nil, nil) == node.body
- add_error "remove empty helpers", node.file, node.line
+ add_callback :start_module do |module_node|
+ if "ApplicationHelper" != module_node.module_name.to_s && empty_body?(module_node)
+ add_error "remove empty helpers"
end
end
+
+ protected
+ def empty_body?(module_node)
+ s(:bodystmt, s(:stmts_add, s(:stmts_new), s(:void_stmt)), nil, nil, nil) == module_node.body
+ end
end
end
end
View
2 lib/rails_best_practices/version.rb
@@ -1,4 +1,4 @@
# encoding: utf-8
module RailsBestPractices
- VERSION = "1.11.1"
+ VERSION = "1.12.0"
end
View
1 rails_best_practices.yml
@@ -12,6 +12,7 @@ MoveCodeIntoModelCheck: { use_count: 2 }
MoveFinderToNamedScopeCheck: { }
MoveModelLogicIntoModelCheck: { use_count: 4 }
NeedlessDeepNestingCheck: { nested_count: 2 }
+NotRescueExceptionCheck: { }
NotUseDefaultRouteCheck: { }
NotUseTimeAgoInWordsCheck: { }
OveruseRouteCustomizationsCheck: { customize_count: 3 }
View
18 spec/rails_best_practices/analyzer_spec.rb
@@ -43,23 +43,5 @@ module RailsBestPractices
result.should == ["app/models/user.rb:10 - law of demeter".red, "app/models/post.rb:100 - use query attribute".red, "\nPlease go to http://rails-bestpractices.com to see more useful Rails Best Practices.".green, "\nFound 2 warnings.".red].join("\n") + "\n"
end
end
-
- describe "errors_filter_block" do
- it "should call errors_filter_block after analyze" do
- analyzer = Analyzer.new("path", "silent" => true)
- analyzer.errors_filter_block = lambda { |errors| errors.each { |error| error.highlight = true } }
- analyzer.stub(:expand_dirs_to_files).and_return([])
- analyzer.stub(:file_sort).and_return([])
- analyzer.stub(:file_ignore).and_return([])
- analyzer.stub(:process)
- runner = stub
- runner.stub(:color=)
- errors = [Core::Error.new, Core::Error.new]
- Core::Runner.stub(:new).and_return(runner)
- runner.stub(:errors).and_return(errors)
- analyzer.analyze
- analyzer.send(:errors).should be_all(&:highlight)
- end
- end
end
end
View
4 spec/rails_best_practices/core/error_spec.rb
@@ -2,10 +2,6 @@
module RailsBestPractices::Core
describe Error do
- it "should have highlight with false by default" do
- Error.new.highlight.should == false
- end
-
it "should return error with filename, line number and message" do
Error.new(
filename: "app/models/user.rb",
View
18 spec/rails_best_practices/reviews/always_add_db_index_review_spec.rb
@@ -153,6 +153,24 @@ module Reviews
runner.should have(0).errors
end
+ it "should not always add db index with t.index" do
+ # e.g. schema_plus creates indices like this https://github.com/lomba/schema_plus
+ content = <<-EOF
+ ActiveRecord::Schema.define(version: 20100603080629) do
+ create_table "comments", force: true do |t|
+ t.string "content"
+ t.integer "post_id"
+ t.index ["post_id"], :name => "index_comments_on_post_id"
+ end
+ create_table "posts", force: true do |t|
+ end
+ end
+ EOF
+ runner.review('db/schema.rb', content)
+ runner.after_review
+ runner.should have(0).errors
+ end
+
it "should not always add db index with only _type column" do
content = <<-EOF
ActiveRecord::Schema.define(version: 20100603080629) do
View
95 spec/rails_best_practices/reviews/not_rescue_exception_spec.rb
@@ -0,0 +1,95 @@
+require 'spec_helper'
+
+module RailsBestPractices
+ module Reviews
+ describe NotRescueExceptionReview do
+ let(:runner) { Core::Runner.new(reviews: NotRescueExceptionReview.new) }
+
+ describe "not_rescue_exception" do
+ it "should not rescue exception in method rescue with named var" do
+ content =<<-EOF
+ def my_method
+ do_something
+ rescue Exception => e
+ logger.error e
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(1).errors
+ runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:3 - not rescue Exception"
+ end
+
+ it "should not rescue exception in method rescue without named var" do
+ content =<<-EOF
+ def my_method
+ do_something
+ rescue Exception
+ logger.error $!
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(1).errors
+ runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:3 - not rescue Exception"
+ end
+
+ it "should not rescue exception in block rescue with named var" do
+ content =<<-EOF
+ def my_method
+ begin
+ do_something
+ rescue Exception => e
+ logger.error e
+ end
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(1).errors
+ runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:4 - not rescue Exception"
+ end
+
+ it "should not rescue exception in block rescue without named var" do
+ content =<<-EOF
+ def my_method
+ begin
+ do_something
+ rescue Exception
+ logger.error $!
+ end
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(1).errors
+ runner.errors[0].to_s.should == "app/helpers/posts_helper.rb:4 - not rescue Exception"
+ end
+
+ it "should allow rescue implicit StandardError in block rescue without named var" do
+ content =<<-EOF
+ def my_method
+ begin
+ do_something
+ rescue
+ logger.error $!
+ end
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(0).errors
+ end
+
+ it "should allow rescue explicit StandardError in block rescue without named var" do
+ content =<<-EOF
+ def my_method
+ begin
+ do_something
+ rescue StandardError
+ logger.error $!
+ end
+ end
+ EOF
+ runner.review('app/helpers/posts_helper.rb', content)
+ runner.should have(0).errors
+ end
+ end
+ end
+ end
+end
View
22 spec/rails_best_practices/reviews/protect_mass_assignment_review_spec.rb
@@ -95,13 +95,23 @@ class User < ActiveRecord::Base
end
it "should not protect mass assignment if checking non ActiveRecord::Base inherited model" do
- content =<<-EOF
- class User < Person
- end
- EOF
- runner.review('app/models/user.rb', content)
- runner.should have(0).errors
+ content =<<-EOF
+ class User < Person
+ end
+ EOF
+ runner.review('app/models/user.rb', content)
+ runner.should have(0).errors
+ end
+
+ it "should not protect mass assignment for strong_parameters" do
+ content =<<-EOF
+ class User < ActiveRecord::Base
+ include ActiveModel::ForbiddenAttributesProtection
end
+ EOF
+ runner.review('app/models/user.rb', content)
+ runner.should have(0).errors
+ end
end
end
end
View
9 spec/rails_best_practices/reviews/remove_empty_helpers_review_spec.rb
@@ -26,6 +26,15 @@ def post_link(post)
runner.review('app/helpers/posts_helper.rb', content)
runner.should have(0).errors
end
+
+ it "should not remove empty application_helper" do
+ content =<<-EOF
+ module ApplicationHelper
+ end
+ EOF
+ runner.review('app/helpers/application_helper.rb', content)
+ runner.should have(0).errors
+ end
end
end
end

No commit comments for this range

Something went wrong with that request. Please try again.