Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration for only posting warnings to added/modified lines #89

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 24 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
Pronto runner for [RuboCop](https://github.com/bbatsov/rubocop), ruby code
analyzer. [What is Pronto?](https://github.com/prontolabs/pronto)

- [Configuration](#configuration)
- [Usage](#usage)
- [Suggestions](#suggestions)
- [Only patched lines](#only-patched-lines)
- [RuboCop versions](#rubocop-versions)

## Configuration

Configuring RuboCop via `.rubocop.yml` will work just fine with
Expand All @@ -26,6 +32,12 @@ rubocop:

# Enable suggestions
suggestions: true

# Only report warnings on added/modified lines of code
# You can provide a number for a range to catch warnings on lines that were not modified
only_patched_lines:
enabled: false # default
range: 0 # default
```

## Suggestions
Expand All @@ -39,10 +51,21 @@ For example:

![GitHub screenshot with suggestion](https://user-images.githubusercontent.com/132/50402757-1bd75b80-0799-11e9-809f-8b8a23ed33f6.png)

## Only patched lines

When `only_patched_lines` is enabled, Rubocop warnings that start outside of the patched code will be ignored.
For example, if you add a method to a class with too many lines, the warning at the class level will not apply.

This can be useful for legacy applications with a lot of RuboCop warnings, where you want to focus on the new code.

When increasing the range, you will also catch warnings on lines that were not modified but are within the range of the modified lines.

For example, if you set `range: 1`, you will catch warnings starting before the patched lines, but only if they are within 1 line.
Phillita marked this conversation as resolved.
Show resolved Hide resolved

## RuboCop versions

If you need to use RuboCop v0.84.0 or v0.85.x, you'll need to ensure that
you've also need to add `gem 'rubocop-ast', '< 0.7.0'` to your Gemfile as
you've also added `gem 'rubocop-ast', '< 0.7.0'` to your Gemfile as
these were the first versions to use rubocop-ast, and unfortunately the
dependency was loose enough that rubocop-ast versions >= 0.7.0 were allowed,
which causes `require 'rubocop'` to fail with
Expand Down
19 changes: 17 additions & 2 deletions lib/pronto/rubocop/patch_cop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,11 @@ def offenses
end

def offense_includes?(offense, line_number)
offense_range = (offense.location.first_line..offense.location.last_line)
offense_range.include?(line_number)
if only_patched_lines?
(offense.location.first_line..offense.location.first_line + only_patched_lines_range).include?(line_number)
else
(offense.location.first_line..offense.location.last_line).include?(line_number)
end
end

def team
Expand All @@ -91,6 +94,18 @@ def first_relevant_message(patch, offense)

OffenseLine.new(self, offense, offending_line).message
end

def only_patched_lines
runner.pronto_rubocop_config.fetch('only_patched_lines', {})
end

def only_patched_lines?
@only_patched_lines ||= only_patched_lines.fetch('enabled', false)
end

def only_patched_lines_range
@only_patched_lines_range ||= only_patched_lines.fetch('range', 0)
end
end
end
end
119 changes: 103 additions & 16 deletions spec/pronto/rubocop/patch_cop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,40 +4,127 @@

describe Pronto::Rubocop::PatchCop do
let(:patch_cop) { described_class.new(patch, runner) }
let(:patch) do
double :patch, new_file_full_path: 'example.rb', added_lines: [line]
end
let(:patch) { instance_double Pronto::Git::Patch, new_file_full_path: 'example.rb', added_lines: [line] }
let(:line) { double :line, new_lineno: 42 }
let(:runner) { double :runner }
let(:runner) { instance_double Pronto::Rubocop, pronto_rubocop_config: config }
let(:config) { {} }
let(:ast) { double :ast, each_node: nil }
let(:processed_source) { double :processed_source, ast: ast }
let(:team) { double :team, inspect_file: [offense] }
let(:rubocop_processed_source) { instance_double RuboCop::ProcessedSource, ast: ast }
let(:team) { instance_double RuboCop::Cop::Team, inspect_file: [offense] }
let(:offense_location) { double :location, first_line: 42, last_line: 43 }
let(:offense) { double :offense, disabled?: false, location: offense_location }
let(:offense_line) { double :offense_line, message: 'Err' }
let(:offense) { instance_double RuboCop::Cop::Offense, disabled?: false, location: offense_location }
let(:offense_line) { instance_double Pronto::Rubocop::OffenseLine, message: 'Err' }

before do
allow(RuboCop::ProcessedSource).to receive(:from_file) { processed_source }
allow(RuboCop::ProcessedSource).to receive(:from_file) { rubocop_processed_source }
allow(RuboCop::Cop::Team).to receive(:new) { team }
allow(Pronto::Rubocop::OffenseLine).to receive(:new) { offense_line }
end

describe '#processed_source' do
it do
expect(patch_cop.processed_source).to eq(processed_source)
end
subject(:processed_source) { patch_cop.processed_source }

it { expect(processed_source).to eq(rubocop_processed_source) }
end

describe '#messages' do
it do
expect(patch_cop.messages).to eq(['Err'])
subject(:messages) { patch_cop.messages }

it { expect(messages).to eq(['Err']) }

context 'when there is an error only on the patch line' do
let(:offense_location) { double :location, first_line: 42, last_line: 42 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error including the patch, but not starting inside it' do
let(:offense_location) { double :location, first_line: 40, last_line: 43 }

it do
expect(patch_cop.messages).to eq(['Err'])
it { expect(messages).to eq(['Err']) }
end

context 'when there is an error excluding the patch' do
let(:offense_location) { double :location, first_line: 40, last_line: 41 }

it { expect(messages).to eq([]) }
end

context 'when rubocop config for only_patched_lines is not enabled' do
let(:config) { { 'only_patched_lines' => { 'enabled' => false } } }

it { expect(messages).to eq(['Err']) }

context 'when there is an error only on the patch line' do
let(:offense_location) { double :location, first_line: 42, last_line: 42 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error including the patch, but not starting inside it' do
let(:offense_location) { double :location, first_line: 40, last_line: 43 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error excluding the patch' do
let(:offense_location) { double :location, first_line: 40, last_line: 41 }

it { expect(messages).to eq([]) }
end
end

context 'when rubocop config for only_patched_lines is enabled' do
let(:config) { { 'only_patched_lines' => { 'enabled' => true } } }

it { expect(messages).to eq(['Err']) }

context 'when there is an error only on the patch line' do
let(:offense_location) { double :location, first_line: 42, last_line: 42 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error including the patch, but not starting inside it' do
let(:offense_location) { double :location, first_line: 40, last_line: 43 }

it { expect(messages).to eq([]) }
end

context 'when there is an error excluding the patch' do
let(:offense_location) { double :location, first_line: 40, last_line: 41 }

it { expect(messages).to eq([]) }
end
end

context 'when rubocop config for only_patched_lines is enabled with a range of 5' do
let(:config) { { 'only_patched_lines' => { 'enabled' => true, 'range' => 5 } } }

it { expect(messages).to eq(['Err']) }

context 'when there is an error only on the patch line' do
let(:offense_location) { double :location, first_line: 42, last_line: 42 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error including the patch, but not starting inside it' do
let(:offense_location) { double :location, first_line: 40, last_line: 43 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error excluding the patch' do
let(:offense_location) { double :location, first_line: 40, last_line: 41 }

it { expect(messages).to eq(['Err']) }
end

context 'when there is an error excluding the patch outside the range' do
let(:offense_location) { double :location, first_line: 30, last_line: 41 }

it { expect(messages).to eq([]) }
end
end
end
Expand Down