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

Use auto generated Rubocop config #2418

Merged
merged 11 commits into from
Mar 30, 2016
Merged
5 changes: 5 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
inherit_from:
- .rubocop_todo.yml
AllCops:
Exclude:
- 'tmp/**/*'
536 changes: 536 additions & 0 deletions .rubocop_todo.yml

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion app/models/ukba_country.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

class UkbaCountry < OpenStruct
def self.all
@countries ||= YAML.load_file(Rails.root.join('lib', 'data', 'ukba_additional_countries.yml')).map {|c| self.new(c) }
@countries ||= YAML.load_file(Rails.root.join('lib', 'data', 'ukba_additional_countries.yml')).map { |c| self.new(c) }
end
end
2 changes: 1 addition & 1 deletion app/models/world_location.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,6 @@ def organisations
end

def fco_organisation
self.organisations.find {|o| o.fco_sponsored? }
self.organisations.find { |o| o.fco_sponsored? }
end
end
2 changes: 1 addition & 1 deletion app/models/worldwide_organisation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def all_offices
end

def fco_sponsored?
@data.sponsors.any? {|s| s.details.acronym == "FCO" }
@data.sponsors.any? { |s| s.details.acronym == "FCO" }
end

def offices_with_service(service_title)
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/country_select_question_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class CountrySelectQuestionPresenter < QuestionPresenter
def response_label(value)
options.find {|option| option.slug == value}.name
options.find { |option| option.slug == value }.name
end

def options
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/date_question_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def end_date
@node.range == false ? 3.years.from_now : @node.range.end
end

private
private

def only_display_day_and_month?(value)
value.year.zero?
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/multiple_choice_question_presenter.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
class MultipleChoiceQuestionPresenter < QuestionPresenter
def response_label(value)
options.find {|option| option.value == value}.label
options.find { |option| option.value == value }.label
end
end
2 changes: 1 addition & 1 deletion config/initializers/flow_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
if Rails.env.production?
FLOW_REGISTRY_OPTIONS = {}
else
FLOW_REGISTRY_OPTIONS = {show_drafts: true}
FLOW_REGISTRY_OPTIONS = { show_drafts: true }
end

# Uncomment the following to run smartanswers with the test flows instead of the real ones
Expand Down
16 changes: 12 additions & 4 deletions doc/rubocop.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
# Rubocop

We're using the govuk-lint Gem to include Rubocop and the GOV.UK styleguide rules in the project.
We're using the `govuk-lint` Gem to include Rubocop and the GOV.UK styleguide rules in the project.

### Jenkins

We run Rubocop as part of the test suite executed on Jenkins (see jenkins.sh). Rubocop only tests for violations in files introduced in the branch being tested. This should prevent us from introducing new violations without us having to first fix all existing violations.
We run `govuk-lint-ruby` as part of the test suite executed on Jenkins (see jenkins.sh). Behind the scenes this runs Rubocop with a set of cops defined in the `govuk-lint` gem. However, because `govuk-lint` was retro-fitted to the application relatively recently, there were a large number of cop violations in the codebase.

We've dealt with this by generating a project-specific `.rubocop_todo.yml` file using `govuk-lint-ruby --auto-gen-config --exclude-limit 99999` which [excludes existing violations on a per-cop basis][1]. The limit was set to `99999` to minimise the number of cops that are disabled entirely - most are only disabled for a specific set of files.

The generated `.rubocop_todo.yml` file is included into the configuration via a local `.rubocop.yml` file using an `inherit_from` key. This means that the exclusions in `.rubocop_todo.yml` are used for all `govuk-lint-ruby` commands by default. Note that the `.rubocop.yml` file does not override the cop definitions in the `govuk-lint` gem - it is merely a way to add extra configuration.

Using this approach should prevent us from introducing new violations into the project without needing to fix all the existing violations first. However, in the long run, the idea is for us to gradually remove these exclusions to bring the project fully in line with the default `govuk-lint-ruby` configuration. Ultimately we will be able to remove the `.rubocop_todo.yml` & `.rubocop.yml` files and just rely on the default `govuk-lint` configuration.

Note that as per the header comment in `.rubocop_todo.yml`, changes in the inspected code, or installation of new versions of RuboCop, may require `.rubocop_todo.yml` to be generated again.

### Running locally

Testing for violations in the entire codebase:
Testing for violations in the entire codebase (used by `jenkins.sh`):

```bash
$ govuk-lint-ruby
Expand All @@ -26,4 +34,4 @@ Testing for violations in code staged and committed locally that's not present i
$ govuk-lint-ruby --diff --cached
```

NOTE. This is mostly useful for Jenkins as we first merge, but don't commit, changes from master before running the test suite.
[1]: https://github.com/bbatsov/rubocop/blob/master/README.md#automatically-generated-configuration
1 change: 0 additions & 1 deletion jenkins.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export DISPLAY=:99

if [ -z "$RUN_REGRESSION_TESTS" ]; then
bundle exec govuk-lint-ruby \
--diff --cached \
--format clang

RAILS_ENV=test TEST_COVERAGE=true bundle exec rake test
Expand Down
2 changes: 1 addition & 1 deletion lib/data/state_pension_date_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def find_date
private

def run(pension_dates)
pension_dates.find {|p| p.match?(dob, gender)}
pension_dates.find { |p| p.match?(dob, gender) }
end

# Handle the case where the person's d.o.b. is 29th Feb
Expand Down
2 changes: 1 addition & 1 deletion lib/graph_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ def initialize(flow)
end

def labels
Hash[@flow.nodes.map { |node| [node.name, graph_label_text(node)]}]
Hash[@flow.nodes.map { |node| [node.name, graph_label_text(node)] }]
end

def adjacency_list
Expand Down
2 changes: 1 addition & 1 deletion lib/graphviz_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def label_lines
style: "filled"
)
end
attribute_clause = attrs.map {|k, v| "#{k}=\"#{v}\""}.join(' ')
attribute_clause = attrs.map { |k, v| "#{k}=\"#{v}\"" }.join(' ')
%Q{#{normalize_name(name)} [#{attribute_clause}]}
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/commodity_code_calculator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def has_commodity_code?
commodity_code != 'X'
end

private
private

def glucose_sucrose_index
starch_glucose_to_sucrose[@starch_glucose_weight][@sucrose_weight]
Expand Down
4 changes: 2 additions & 2 deletions lib/smart_answer/calculators/country_name_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class CountryNameFormatter
"usa" => "the USA"
}

def definitive_article(country, capitalized=false)
def definitive_article(country, capitalized = false)
result = country_name(country)
if requires_definite_article?(country)
result = capitalized ? "The #{result}" : "the #{result}"
Expand All @@ -32,7 +32,7 @@ def friendly_name(country)
FRIENDLY_COUNTRY_NAME[country]
end

private
private

def country_name(country)
WorldLocation.find(country).name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ def included_country?
postcode_within?(VALID_COUNTRIES, 'country_name')
end

private
private

def postcode_within?(included_areas, key_name)
areas_for_postcode.select {|a| included_areas.include?(a[key_name]) }.any?
areas_for_postcode.select { |a| included_areas.include?(a[key_name]) }.any?
end

def areas_for_postcode
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/marriage_abroad_data_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def ss_alt_fees_table_country?(country_slug, calculator)
SS_ALT_FEES_TABLE_COUNTRY.include?(country_slug) ||
(SS_ALT_FEES_TABLE_OR_OUTCOME_GROUP_A.include?(country_slug) && calculator.partner_british?) ||
(SS_ALT_FEES_TABLE_OR_OUTCOME_GROUP_B.include?(country_slug) && calculator.partner_is_not_national_of_ceremony_country?) &&
(%w(cambodia vietnam).exclude?(country_slug))
(%w(cambodia vietnam).exclude?(country_slug))
end

def ss_marriage_not_possible?(country_slug, calculator)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def smp_lel
end
end

private
private

def due_date_before_7th_april_2013?
@due_date < Date.parse("7th April 2013")
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/minimum_wage_calculator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ def any_overtime_hours_worked?
overtime_hours > 0
end

protected
protected

def weekly_multiplier
(@pay_frequency.to_f / 7).round(3)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def pro_rata_taxable_profit
(profit_per_day * award_period.number_of_days).floor
end

private
private

def accounting_year_end_date_in_the_tax_year_that_tax_credits_award_ends
accounting_date = accounts_end_month_and_day.change(year: tax_year.begins_on.year)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ def in_2015_2016_fin_year?(date)
(Date.new(2015, 05, 06)..Date.new(2016, 05, 05)).cover?(date)
end

private
private

def saturday_before(date)
(date - date.wday) - 1.day
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/plan_adoption_leave.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def period_of_additional_leave
period_of_ordinary_leave && weeks_later(period_of_ordinary_leave, 26)
end

private
private
def weeks_later(range, weeks)
(range.first + weeks * 7) .. (range.last + weeks * 7)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/plan_maternity_leave.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def period_of_additional_leave
period_of_ordinary_leave && weeks_later(period_of_ordinary_leave, 26)
end

private
private
def weeks_later(range, weeks)
(range.first + weeks * 7) .. (range.last + weeks * 7)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/self_assessment_penalties.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def late_payment_penalty
end
end

private
private
def overdue_filing_days
(filing_date - filing_deadline).to_i
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def lump_sum_and_age(dob, weekly_amount, gender)
age = age_at_date(dob, TOPUP_START_DATE)
(TOPUP_START_DATE.year..TOPUP_END_DATE.year).each do |_|
break if birthday_after_topup_end?(dob, age)
rows << {amount: lump_sum_amount(age, weekly_amount), age: age} if age >= retirement_age(gender)
rows << { amount: lump_sum_amount(age, weekly_amount), age: age } if age >= retirement_age(gender)
age += 1
end
rows
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/calculators/vat_payment_deadlines.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ def funds_received_by
end
end

private
private

def end_of_month_after(date)
1.month.since(date).end_of_month
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/erb_renderer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def erb_template_path
@template_directory.join(erb_template_name)
end

private
private

def erb_template_name
"#{@template_name}.govspeak.erb"
Expand Down
6 changes: 3 additions & 3 deletions lib/smart_answer/flow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,11 @@ def questions
end

def node_exists?(node_or_name)
@nodes.any? {|n| n.name == node_or_name.to_sym }
@nodes.any? { |n| n.name == node_or_name.to_sym }
end

def node(node_or_name)
@nodes.find {|n| n.name == node_or_name.to_sym } or raise "Node '#{node_or_name}' does not exist"
@nodes.find { |n| n.name == node_or_name.to_sym } or raise "Node '#{node_or_name}' does not exist"
end

def start_state
Expand Down Expand Up @@ -131,7 +131,7 @@ def normalize_responses(responses)

class InvalidStatus < StandardError; end

private
private

def add_question(klass, name, options = {}, &block)
add_node klass.new(self, name, options, &block)
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/question/checkbox.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def parse_input(raw_input)
end

def to_response(input)
input.split(',').reject {|v| v == NONE_OPTION }
input.split(',').reject { |v| v == NONE_OPTION }
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/smart_answer/question/country_select.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def country_in(countries)
end

def valid_option?(option)
options.map {|v| v.slug}.include? (option.to_s)
options.map { |v| v.slug }.include? (option.to_s)
end

def parse_input(raw_input)
Expand All @@ -40,7 +40,7 @@ def load_countries
countries = countries.reject { |c| @exclude_countries.include?(c.slug) }
end
if @additional_countries
countries = (countries + @additional_countries).sort {|a, b| a.name <=> b.name }
countries = (countries + @additional_countries).sort { |a, b| a.name <=> b.name }
end
countries
end
Expand Down
26 changes: 14 additions & 12 deletions lib/smart_answer/question/date.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,23 +52,25 @@ def range
end

def parse_input(input)
date = case input
date = begin
case input
when Hash, ActiveSupport::HashWithIndifferentAccess
input = input.symbolize_keys
year_month_and_day = [
default_year || input[:year],
default_month || input[:month],
default_day || input[:day],
]
raise InvalidResponse unless year_month_and_day.all?(&:present?)
::Date.new(*year_month_and_day.map(&:to_i))
input = input.symbolize_keys
year_month_and_day = [
default_year || input[:year],
default_month || input[:month],
default_day || input[:day],
]
raise InvalidResponse unless year_month_and_day.all?(&:present?)
::Date.new(*year_month_and_day.map(&:to_i))
when String
::Date.parse(input)
::Date.parse(input)
when ::Date
input
input
else
raise InvalidResponse, "Bad date", caller
raise InvalidResponse, "Bad date", caller
end
end
validate_input(date) if @validate_in_range
date
rescue ArgumentError => e
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer/state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def save_input_as(name)
__send__ "#{name}=", responses.last
end

private
private

def initialize_copy(orig)
super
Expand Down
2 changes: 1 addition & 1 deletion lib/smart_answer_files.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def empty?
existing_paths.empty?
end

private
private

def relative_paths
@relative_paths ||= all_paths.collect do |path|
Expand Down
8 changes: 4 additions & 4 deletions lib/smart_answer_flows/energy-grants-calculator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def define
validate(:error_perm_prop_house) { |r| ! r.include?('permission,property,social_housing') }
validate(:error_prop_house) { |r| ! r.include?('property,social_housing') }
validate(:error_perm_prop) { |r| ! r.include?('permission,property') }
validate(:error_perm_house) { |r| ! r.include?('permission,social_housing')}
validate(:error_perm_house) { |r| ! r.include?('permission,social_housing') }

next_node do
question :date_of_birth? # Q3
Expand Down Expand Up @@ -163,9 +163,9 @@ def define
response == 'esa' ||
response == 'working_tax_credit' ||
response.include?('universal_credit') ||
%w{child_tax_credit esa income_support jsa pension_credit}.all? {|key| response.include? key} ||
%w{child_tax_credit esa income_support pension_credit}.all? {|key| response.include? key} ||
%w{child_tax_credit esa jsa pension_credit}.all? {|key| response.include? key}
%w{child_tax_credit esa income_support jsa pension_credit}.all? { |key| response.include? key } ||
%w{child_tax_credit esa income_support pension_credit}.all? { |key| response.include? key } ||
%w{child_tax_credit esa jsa pension_credit}.all? { |key| response.include? key }
end

next_node do |response|
Expand Down
Loading