From f4f401d56f4c21b3a4ff515e0cdb0ee9ca432a37 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 22 Nov 2017 08:44:29 +0200 Subject: [PATCH] Make config loading more flexible --- README.md | 35 +++--- config/ameba.yml | 4 +- spec/ameba/base_spec.cr | 28 +++++ spec/ameba/rule/duplicated_when_spec.cr | 2 +- spec/ameba/rule/large_numbers_spec.cr | 9 ++ spec/ameba/rule/line_length_spec.cr | 9 ++ spec/ameba/rule/percent_arrays_spec.cr | 16 +++ spec/ameba/rule/redundant_begin_spec.cr | 2 +- spec/spec_helper.cr | 4 + src/ameba/config.cr | 119 +++++++++--------- src/ameba/rule/base.cr | 10 +- src/ameba/rule/comparison_to_boolean.cr | 5 + src/ameba/rule/constant_names.cr | 4 + src/ameba/rule/debugger_statement.cr | 4 + src/ameba/rule/duplicated_when.cr | 6 +- src/ameba/rule/empty_ensure.cr | 4 + src/ameba/rule/empty_expression.cr | 4 + src/ameba/rule/hash_duplicated_key.cr | 4 + src/ameba/rule/large_numbers.cr | 5 +- src/ameba/rule/line_length.cr | 6 +- src/ameba/rule/literal_in_condition.cr | 5 + src/ameba/rule/literal_in_interpolation.cr | 4 + src/ameba/rule/method_names.cr | 4 + .../rule/negated_conditions_in_unless.cr | 4 + src/ameba/rule/percent_arrays.cr | 7 +- src/ameba/rule/predicate_name.cr | 4 + src/ameba/rule/redundant_begin.cr | 5 +- src/ameba/rule/trailing_blank_lines.cr | 4 + src/ameba/rule/trailing_whitespace.cr | 4 + src/ameba/rule/type_names.cr | 4 + src/ameba/rule/unless_else.cr | 4 + src/ameba/rule/variable_names.cr | 4 + src/ameba/runner.cr | 2 +- 33 files changed, 243 insertions(+), 92 deletions(-) create mode 100644 spec/ameba/base_spec.cr diff --git a/README.md b/README.md index 19d99a0f..9a9311df 100644 --- a/README.md +++ b/README.md @@ -108,35 +108,32 @@ Each rule is enabled by default, even if you remove it from the config file. ## Writing a new Rule -Adding a new rule is as simple as inheriting from `Rule::Base` struct and implementing -your logic to detect a problem: +Adding a new rule is as simple as inheriting from `Ameba::Rule::Base` struct and implementing +a logic to detect a problem in the source file: ```crystal -struct DebuggerStatement < Rule::Base +struct MySuperRule < Ameba::Rule::Base # This is a required method to be implemented by the rule. - # Source will be passed here. If rule finds an issue in this source, - # it reports an error: - # - # source.error rule, line_number, message + # Source will be passed here. If rule detects an issue in the source, + # it reports an error: + # + # source.error rule, location, message # def test(source) - # This line deletegates verification to a particular callback in the AST visitor. - AST::Visitor.new self, source - end - - # This method is called once the visitor finds a needed node. - def test(source, node : Crystal::Call) - # It reports an error, if there is `debugger` method call - # without arguments and a receiver. That's it, somebody forgot - # to remove a debugger statement. - return unless node.name == "debugger" && node.args.empty? && node.obj.nil? - - source.error self, node.location, "Possible forgotten debugger statement detected" + # TODO: test source end end ``` +As soon as a custom rule is defined, it becomes available in a full set of rules +executed by default and also can be configured via config file: + +```yaml +MySuperRule: + Enabled: false +``` + ## Credits & inspirations - [Crystal Language](crystal-lang.org) diff --git a/config/ameba.yml b/config/ameba.yml index 934194dd..657eb62f 100644 --- a/config/ameba.yml +++ b/config/ameba.yml @@ -1,6 +1,6 @@ ComparisonToBoolean: # Disallows comparison to booleans in conditions. - Enabled: true + Enabled: false ConstantNames: # Enforces constant names to be in a screaming case. @@ -33,7 +33,7 @@ LargeNumbers: LineLength: # Disallows lines longer that MaxLength number of symbols. - Enabled: true + Enabled: false MaxLength: 80 LiteralInCondition: diff --git a/spec/ameba/base_spec.cr b/spec/ameba/base_spec.cr new file mode 100644 index 00000000..3e28b62e --- /dev/null +++ b/spec/ameba/base_spec.cr @@ -0,0 +1,28 @@ +require "../spec_helper" + +module Ameba::Rule + struct NoProperties < Rule::Base + def test(source) + end + end + + describe Base do + context "properties" do + subject = DummyRule.new + + it "is enabled by default" do + subject.enabled.should be_true + end + + it "has a description property" do + subject.description.should_not be_nil + end + end + + describe "when a rule does not have defined properties" do + it "is enabled by default" do + NoProperties.new.enabled.should be_true + end + end + end +end diff --git a/spec/ameba/rule/duplicated_when_spec.cr b/spec/ameba/rule/duplicated_when_spec.cr index eef96b7e..e7dd1bb2 100644 --- a/spec/ameba/rule/duplicated_when_spec.cr +++ b/spec/ameba/rule/duplicated_when_spec.cr @@ -112,7 +112,7 @@ module Ameba::Rule error = s.errors.first error.rule.should_not be_nil error.location.to_s.should eq "source.cr:2:9" - error.message.should eq "Duplicated when conditions in case." + error.message.should eq "Duplicated when conditions in case" end end end diff --git a/spec/ameba/rule/large_numbers_spec.cr b/spec/ameba/rule/large_numbers_spec.cr index 6af4643d..8e1390b8 100644 --- a/spec/ameba/rule/large_numbers_spec.cr +++ b/spec/ameba/rule/large_numbers_spec.cr @@ -116,5 +116,14 @@ module Ameba error.location.to_s.should eq "source.cr:2:10" error.message.should match /1_200_000/ end + + context "properties" do + it "allows to configure integer min digits" do + s = Source.new %q(1200000) + rule = Rule::LargeNumbers.new + rule.int_min_digits = 10 + rule.catch(s).should be_valid + end + end end end diff --git a/spec/ameba/rule/line_length_spec.cr b/spec/ameba/rule/line_length_spec.cr index ab17ee9b..4c6ae563 100644 --- a/spec/ameba/rule/line_length_spec.cr +++ b/spec/ameba/rule/line_length_spec.cr @@ -29,5 +29,14 @@ module Ameba::Rule error.location.to_s.should eq "source.cr:1:81" error.message.should eq "Line too long" end + + context "properties" do + it "allows to configure max length of the line" do + source = Source.new long_line + rule = LineLength.new + rule.max_length = long_line.size + rule.catch(source).should be_valid + end + end end end diff --git a/spec/ameba/rule/percent_arrays_spec.cr b/spec/ameba/rule/percent_arrays_spec.cr index 184d1a57..9b3ffc97 100644 --- a/spec/ameba/rule/percent_arrays_spec.cr +++ b/spec/ameba/rule/percent_arrays_spec.cr @@ -65,5 +65,21 @@ module Ameba::Rule "Symbols `,\"` may be unwanted in %w array literals" ) end + + context "properties" do + it "allows to configure string_array_unwanted_symbols" do + rule = PercentArrays.new + rule.string_array_unwanted_symbols = "," + s = Source.new %( %w("one") ) + rule.catch(s).should be_valid + end + + it "allows to configure symbol_array_unwanted_symbols" do + rule = PercentArrays.new + rule.symbol_array_unwanted_symbols = "," + s = Source.new %( %i(:one) ) + rule.catch(s).should be_valid + end + end end end diff --git a/spec/ameba/rule/redundant_begin_spec.cr b/spec/ameba/rule/redundant_begin_spec.cr index 48b1bd43..4e0f4061 100644 --- a/spec/ameba/rule/redundant_begin_spec.cr +++ b/spec/ameba/rule/redundant_begin_spec.cr @@ -207,7 +207,7 @@ module Ameba::Rule error = s.errors.first error.rule.should_not be_nil error.location.to_s.should eq "source.cr:2:9" - error.message.should eq "Redundant `begin` block detected." + error.message.should eq "Redundant `begin` block detected" end end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index 5a1a9ac0..505d6e7a 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -3,6 +3,10 @@ require "../src/ameba" module Ameba struct DummyRule < Rule::Base + properties do + description : String = "Dummy rule that does nothing." + end + def test(source) end end diff --git a/src/ameba/config.cr b/src/ameba/config.cr index 4263ba6f..c6c0c188 100644 --- a/src/ameba/config.cr +++ b/src/ameba/config.cr @@ -80,66 +80,71 @@ class Ameba::Config Formatter::DotFormatter.new end - # An entity that represents a corresponding configuration for a specific Rule. + # :no_doc: module Rule - # Represents a configuration of a specific Rule. - getter config : YAML::Any? - - # A macro that defines a dsl to define configurable properties. - # - # ``` - # class Configurable - # include Ameba::Config::Rule - # - # prop enabled? = false - # prop max_length = 80 - # prop wildcard = "*" - # end - # ``` - # - macro prop(assign) - # Rule configuration property. - def {{assign.target}} - {% prop_name = assign.target.id.camelcase.gsub(/\?/, "") %} - - {% if assign.value.is_a? NumberLiteral %} - int_prop "{{prop_name}}", {{assign.value}} - {% elsif assign.value.is_a? BoolLiteral %} - bool_prop "{{prop_name}}", {{assign.value}} - {% elsif assign.value.is_a? StringLiteral %} - str_prop "{{prop_name}}", {{assign.value}} + macro properties(&block) + {% definitions = [] of NamedTuple %} + {% if block.body.is_a? Assign %} + {% definitions << {var: block.body.target, value: block.body.value} %} + {% elsif block.body.is_a? TypeDeclaration %} + {% definitions << {var: block.body.var, value: block.body.value, type: block.body.type} %} + {% elsif block.body.is_a? Expressions %} + {% for prop in block.body.expressions %} + {% if prop.is_a? Assign %} + {% definitions << {var: prop.target, value: prop.value} %} + {% elsif prop.is_a? TypeDeclaration %} + {% definitions << {var: prop.var, value: prop.value, type: prop.type} %} + {% end %} {% end %} + {% end %} + + {% properties = {} of MacroId => NamedTuple %} + {% for df in definitions %} + {% name = df[:var].id %} + {% key = name.camelcase.stringify %} + {% value = df[:value] %} + {% type = df[:type] %} + + {% if type == nil %} + {% if value.is_a? BoolLiteral %} + {% type = Bool %} + {% elsif value.is_a? StringLiteral %} + {% type = String %} + {% elsif value.is_a? NumberLiteral %} + {% if value.kind == :i32 %} + {% type = Int32 %} + {% elsif value.kind == :i64 %} + {% type = Int64 %} + {% elsif value.kind == :f32 %} + {% type = Float32 %} + {% elsif value.kind == :f64 %} + {% type = Float64 %} + {% end %} + {% end %} + + {% type = Nil if type == nil %} + {% end %} + + {% properties[name] = {key: key, default: value, type: type} %} + {% end %} + + {% if properties["enabled".id] == nil %} + {% properties["enabled".id] = {key: "Enabled", default: true, type: Bool} %} + {% end %} + + YAML.mapping({{properties}}) + end + + macro included + macro inherited + # allow creating rules without properties + properties {} + + def self.new(config : Ameba::Config? = nil) + yaml = config.try &.subconfig(class_name).try &.to_yaml || "{}" + from_yaml yaml + end end end - - # Creates an instance of a Rule configuration. - # - # ``` - # class Configurable - # include Ameba::Config::Rule - # - # prop enabled? = false - # prop max_length = 80 - # prop wildcard = "*" - # end - # - # Configurable.new config - # ``` - # - def initialize(config = nil) - @config = config.try &.subconfig(name) - end - - protected def int_prop(name, default : Number) - str_prop(name, default).to_i - end - - protected def bool_prop(name, default : Bool) - str_prop(name, default.to_s) == "true" - end - - protected def str_prop(name, default) - config.try &.[name]?.try &.as_s || default - end end end diff --git a/src/ameba/rule/base.cr b/src/ameba/rule/base.cr index b1055b62..5b2fdc55 100644 --- a/src/ameba/rule/base.cr +++ b/src/ameba/rule/base.cr @@ -27,10 +27,6 @@ module Ameba::Rule # that are tested by this rule, it should add an error. abstract def test(source : Source) - # Enabled property indicates whether this rule enabled or not. - # Only enabled rules will be included into the inspection. - prop enabled? = true - def test(source : Source, node : Crystal::ASTNode) # can't be abstract end @@ -59,7 +55,11 @@ module Ameba::Rule # ``` # def name - self.class.name.gsub("Ameba::Rule::", "") + {{@type}}.class_name + end + + protected def self.class_name + name.gsub("Ameba::Rule::", "") end protected def self.subclasses diff --git a/src/ameba/rule/comparison_to_boolean.cr b/src/ameba/rule/comparison_to_boolean.cr index 16edb5e1..4b024c75 100644 --- a/src/ameba/rule/comparison_to_boolean.cr +++ b/src/ameba/rule/comparison_to_boolean.cr @@ -21,6 +21,11 @@ module Ameba::Rule # ``` # struct ComparisonToBoolean < Base + properties do + enabled = false + description = "Disallows comparison to booleans" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/constant_names.cr b/src/ameba/rule/constant_names.cr index 73fbf39b..8a5af645 100644 --- a/src/ameba/rule/constant_names.cr +++ b/src/ameba/rule/constant_names.cr @@ -23,6 +23,10 @@ module Ameba::Rule # ``` # struct ConstantNames < Base + properties do + description = "Enforces constant names to be in screaming case" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/debugger_statement.cr b/src/ameba/rule/debugger_statement.cr index 1256e85d..40403800 100644 --- a/src/ameba/rule/debugger_statement.cr +++ b/src/ameba/rule/debugger_statement.cr @@ -12,6 +12,10 @@ module Ameba::Rule # ``` # struct DebuggerStatement < Base + properties do + description = "Disallows calls to debugger" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/duplicated_when.cr b/src/ameba/rule/duplicated_when.cr index fb87209e..0cfab1e9 100644 --- a/src/ameba/rule/duplicated_when.cr +++ b/src/ameba/rule/duplicated_when.cr @@ -31,6 +31,10 @@ module Ameba::Rule # ``` # struct DuplicatedWhen < Base + properties do + description = "Disallows duplicated when conditions in case" + end + def test(source) AST::Visitor.new self, source end @@ -38,7 +42,7 @@ module Ameba::Rule def test(source, node : Crystal::Case) return unless duplicated_whens?(node.whens) - source.error self, node.location, "Duplicated when conditions in case." + source.error self, node.location, "Duplicated when conditions in case" end private def duplicated_whens?(whens) diff --git a/src/ameba/rule/empty_ensure.cr b/src/ameba/rule/empty_ensure.cr index 1d2f6822..e677a436 100644 --- a/src/ameba/rule/empty_ensure.cr +++ b/src/ameba/rule/empty_ensure.cr @@ -33,6 +33,10 @@ module Ameba::Rule # ``` # struct EmptyEnsure < Base + properties do + description = "Disallows empty ensure statement" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/empty_expression.cr b/src/ameba/rule/empty_expression.cr index a141e1ff..b9e4e091 100644 --- a/src/ameba/rule/empty_expression.cr +++ b/src/ameba/rule/empty_expression.cr @@ -31,6 +31,10 @@ module Ameba::Rule struct EmptyExpression < Base include AST::Util + properties do + description = "Disallows empty expressions" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/hash_duplicated_key.cr b/src/ameba/rule/hash_duplicated_key.cr index 52a59b48..12a9939b 100644 --- a/src/ameba/rule/hash_duplicated_key.cr +++ b/src/ameba/rule/hash_duplicated_key.cr @@ -21,6 +21,10 @@ module Ameba::Rule # ``` # struct HashDuplicatedKey < Base + properties do + description = "Disallows duplicated keys in hash literals" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/large_numbers.cr b/src/ameba/rule/large_numbers.cr index b0144c1c..c6b0105e 100644 --- a/src/ameba/rule/large_numbers.cr +++ b/src/ameba/rule/large_numbers.cr @@ -28,7 +28,10 @@ module Ameba::Rule # ``` # struct LargeNumbers < Base - prop int_min_digits = 5 + properties do + description = "Disallows usage of large numbers without underscore" + int_min_digits = 5 + end def test(source) Tokenizer.new(source).run do |token| diff --git a/src/ameba/rule/line_length.cr b/src/ameba/rule/line_length.cr index 9a6e0186..ff447178 100644 --- a/src/ameba/rule/line_length.cr +++ b/src/ameba/rule/line_length.cr @@ -10,7 +10,11 @@ module Ameba::Rule # ``` # struct LineLength < Base - prop max_length = 80 + properties do + enabled = false + description = "Disallows lines longer than `MaxLength` number of symbols" + max_length = 80 + end def test(source) source.lines.each_with_index do |line, index| diff --git a/src/ameba/rule/literal_in_condition.cr b/src/ameba/rule/literal_in_condition.cr index 8f476f39..fbd0dcb8 100644 --- a/src/ameba/rule/literal_in_condition.cr +++ b/src/ameba/rule/literal_in_condition.cr @@ -23,6 +23,11 @@ module Ameba::Rule struct LiteralInCondition < Base include AST::Util + properties do + description = "Disallows useless conditional statements that contain\ + a literal in place of a variable or predicate function" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/literal_in_interpolation.cr b/src/ameba/rule/literal_in_interpolation.cr index 6c411d95..6f86cad4 100644 --- a/src/ameba/rule/literal_in_interpolation.cr +++ b/src/ameba/rule/literal_in_interpolation.cr @@ -19,6 +19,10 @@ module Ameba::Rule struct LiteralInInterpolation < Base include AST::Util + properties do + description = "Disallows useless string interpolations" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/method_names.cr b/src/ameba/rule/method_names.cr index 053e26ea..46758779 100644 --- a/src/ameba/rule/method_names.cr +++ b/src/ameba/rule/method_names.cr @@ -39,6 +39,10 @@ module Ameba::Rule # ``` # struct MethodNames < Base + properties do + description = "Enforces method names to be in underscored case" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/negated_conditions_in_unless.cr b/src/ameba/rule/negated_conditions_in_unless.cr index 10658097..8a6a8801 100644 --- a/src/ameba/rule/negated_conditions_in_unless.cr +++ b/src/ameba/rule/negated_conditions_in_unless.cr @@ -28,6 +28,10 @@ module Ameba::Rule # ``` # struct NegatedConditionsInUnless < Base + properties do + description = "Disallows negated conditions in unless" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/percent_arrays.cr b/src/ameba/rule/percent_arrays.cr index f967bfee..99c8e588 100644 --- a/src/ameba/rule/percent_arrays.cr +++ b/src/ameba/rule/percent_arrays.cr @@ -25,8 +25,11 @@ module Ameba::Rule # ``` # struct PercentArrays < Base - prop string_array_unwanted_symbols = ",\"" - prop symbol_array_unwanted_symbols = ",:" + properties do + description = "Disallows some unwanted symbols in percent array literals" + string_array_unwanted_symbols = ",\"" + symbol_array_unwanted_symbols = ",:" + end def test(source) error = start_token = nil diff --git a/src/ameba/rule/predicate_name.cr b/src/ameba/rule/predicate_name.cr index 2f2fa4de..619c3205 100644 --- a/src/ameba/rule/predicate_name.cr +++ b/src/ameba/rule/predicate_name.cr @@ -30,6 +30,10 @@ module Ameba::Rule # ``` # struct PredicateName < Base + properties do + description = "Disallows tautological predicate names" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/redundant_begin.cr b/src/ameba/rule/redundant_begin.cr index 2a67d267..4833f9f0 100644 --- a/src/ameba/rule/redundant_begin.cr +++ b/src/ameba/rule/redundant_begin.cr @@ -57,6 +57,9 @@ module Ameba::Rule # struct RedundantBegin < Base include AST::Util + properties do + description = "Disallows redundant begin blocks" + end def test(source) AST::Visitor.new self, source @@ -65,7 +68,7 @@ module Ameba::Rule def test(source, node : Crystal::Def) return unless redundant_begin?(source, node) - source.error self, node.location, "Redundant `begin` block detected." + source.error self, node.location, "Redundant `begin` block detected" end private def redundant_begin?(source, node) diff --git a/src/ameba/rule/trailing_blank_lines.cr b/src/ameba/rule/trailing_blank_lines.cr index ecc188ec..75ef51a3 100644 --- a/src/ameba/rule/trailing_blank_lines.cr +++ b/src/ameba/rule/trailing_blank_lines.cr @@ -9,6 +9,10 @@ module Ameba::Rule # ``` # struct TrailingBlankLines < Base + properties do + description = "Disallows trailing blank lines" + end + def test(source) if source.lines.size > 1 && source.lines[-2, 2].join.strip.empty? source.error self, source.location(source.lines.size, 1), diff --git a/src/ameba/rule/trailing_whitespace.cr b/src/ameba/rule/trailing_whitespace.cr index 02945881..27371d77 100644 --- a/src/ameba/rule/trailing_whitespace.cr +++ b/src/ameba/rule/trailing_whitespace.cr @@ -9,6 +9,10 @@ module Ameba::Rule # ``` # struct TrailingWhitespace < Base + properties do + description = "Disallows trailing whitespaces" + end + def test(source) source.lines.each_with_index do |line, index| next unless line =~ /\s$/ diff --git a/src/ameba/rule/type_names.cr b/src/ameba/rule/type_names.cr index bd6fb970..eefdcdd3 100644 --- a/src/ameba/rule/type_names.cr +++ b/src/ameba/rule/type_names.cr @@ -53,6 +53,10 @@ module Ameba::Rule # ``` # struct TypeNames < Base + properties do + description = "Enforces type names in camelcase manner" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/unless_else.cr b/src/ameba/rule/unless_else.cr index 7bcbd5dd..dba5d1ee 100644 --- a/src/ameba/rule/unless_else.cr +++ b/src/ameba/rule/unless_else.cr @@ -44,6 +44,10 @@ module Ameba::Rule # ``` # struct UnlessElse < Base + properties do + description = "Disallows the use of an `else` block with the `unless`" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/rule/variable_names.cr b/src/ameba/rule/variable_names.cr index 2536e636..815b7a02 100644 --- a/src/ameba/rule/variable_names.cr +++ b/src/ameba/rule/variable_names.cr @@ -32,6 +32,10 @@ module Ameba::Rule # ``` # struct VariableNames < Base + properties do + description = "Enforces variable names to be in underscored case" + end + def test(source) AST::Visitor.new self, source end diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 57f60173..246e1eea 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -93,7 +93,7 @@ module Ameba end private def load_rules(config) - Rule.rules.map { |r| r.new config }.select &.enabled? + Rule.rules.map { |r| r.new config }.select &.enabled end end end