Skip to content

Commit

Permalink
[#69] [#70] Fix bugs related to regex cope
Browse files Browse the repository at this point in the history
Rewrite regex cop code to find regexes usings and then filter already
defined constants.
Additionally added rescue for regex samples with illegal syntax
  • Loading branch information
ignat-z committed Feb 2, 2018
1 parent 1c761bc commit 63071f1
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 6 deletions.
24 changes: 24 additions & 0 deletions DOCUMENTATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,14 @@ puts "hi" if name =~ /john/

```

![](https://placehold.it/10/f03c15/000000?text=+) raises if somewhere in code used regex but defined another const
```ruby

ANOTHER_CONST = /ivan/
puts "hi" if name =~ /john/

```

![](https://placehold.it/10/2cbe4e/000000?text=+) ignores matching constants
```ruby

Expand All @@ -996,6 +1004,14 @@ puts "hi" if name =~ REGEX

```

![](https://placehold.it/10/2cbe4e/000000?text=+) ignores freeze calling
```ruby

REGEX = /john/.freeze
puts "hi" if name =~ REGEX

```

![](https://placehold.it/10/2cbe4e/000000?text=+) ignores named ruby constants
```ruby

Expand All @@ -1010,6 +1026,14 @@ puts "hi" if name =~ /[[:alpha:]]/
name = "john"
puts "hi" if name =~ /.{#{name.length}}/

```

![](https://placehold.it/10/2cbe4e/000000?text=+) rescue dynamic regexes dynamic regexs
```ruby

name = "john"
puts "hi" if name =~ /foo(?=bar)/

```
## Ducalis::RestOnlyCop

Expand Down
39 changes: 33 additions & 6 deletions lib/ducalis/cops/regex_cop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

module Ducalis
class RegexCop < RuboCop::Cop::Cop
include RuboCop::Cop::DefNode

OFFENSE = <<-MESSAGE.gsub(/^ +\|\s/, '').strip
| It's better to move regex to constants with example instead of direct using it. It will allow you to reuse this regex and provide instructions for others.
Expand Down Expand Up @@ -34,22 +36,47 @@ class RegexCop < RuboCop::Cop::Cop
DETAILS = "Available regexes are:
#{SELF_DESCRIPTIVE.map { |name| "`#{name}`" }.join(', ')}"

def on_regexp(node)
return if node.parent.type == :casgn
return if SELF_DESCRIPTIVE.include?(node.source)
return if node.child_nodes.any? { |child_node| child_node.type == :begin }
add_offense(node, :expression, format(OFFENSE, present_node(node)))
DEFAULT_EXAMPLE = 'some_example'

def on_begin(node)
not_defined_regexes(node).each do |regex|
next if SELF_DESCRIPTIVE.include?(regex.source) || const_dynamic?(regex)
add_offense(regex, :expression, format(OFFENSE, present_node(regex)))
end
end

private

def_node_search :const_using, '(regexp $_ ... (regopt))'
def_node_search :const_definition, '(casgn ...)'

def not_defined_regexes(node)
const_using(node).reject do |regex|
defined_as_const?(regex, const_definition(node))
end.map(&:parent)
end

def defined_as_const?(regex, definitions)
definitions.any? { |node| const_using(node).any? { |use| use == regex } }
end

def const_dynamic?(node)
node.child_nodes.any?(&:begin_type?)
end

def present_node(node)
{
constant: node.source,
fixed_string: node.source_range.source_line
.sub(node.source, 'CONST_NAME').lstrip,
example: Regexp.new(node.to_a.first.to_a.first).examples.sample
example: regex_sample(node)
}
end

def regex_sample(node)
Regexp.new(node.to_a.first.to_a.first).examples.sample
rescue RegexpExamples::IllegalSyntaxError
DEFAULT_EXAMPLE
end
end
end
26 changes: 26 additions & 0 deletions spec/ducalis/cops/regex_cop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@
expect(cop).to raise_violation(/puts "hi" if name =~ CONST_NAME/)
end

it 'raises if somewhere in code used regex but defined another const' do
inspect_source(cop, [
'ANOTHER_CONST = /ivan/',
'puts "hi" if name =~ /john/'
])
expect(cop).to raise_violation(/puts "hi"/)
end

it 'ignores matching constants' do
inspect_source(cop, [
'REGEX = /john/',
Expand All @@ -25,6 +33,14 @@
expect(cop).to_not raise_violation
end

it 'ignores freeze calling' do
inspect_source(cop, [
'REGEX = /john/.freeze',
'puts "hi" if name =~ REGEX'
])
expect(cop).to_not raise_violation
end

it 'ignores named ruby constants' do
inspect_source(cop, [
'name = "john"',
Expand All @@ -40,4 +56,14 @@
])
expect(cop).to_not raise_violation
end

it 'rescue dynamic regexes dynamic regexs' do
inspect_source(cop, [
'name = "john"',
'puts "hi" if name =~ /foo(?=bar)/'
])
expect(cop).to raise_violation(
%r{CONST_NAME = /foo\(\?=bar\)/ # "some_example"}
)
end
end

0 comments on commit 63071f1

Please sign in to comment.