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 Rails/RakeEnvironment cop #130

Merged
merged 1 commit into from
Nov 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### New features

* [#123](https://github.com/rubocop-hq/rubocop-rails/pull/123): Add new `Rails/ApplicationController` and `Rails/ApplicationMailer` cops. ([@eugeneius][])
* [#130](https://github.com/rubocop-hq/rubocop-rails/pull/130): Add new `Rails/RakeEnvironment` cop. ([@pocke][])

### Bug fixes

Expand Down Expand Up @@ -95,3 +96,4 @@
[@jas14]: https://github.com/jas14
[@forresty]: https://github.com/forresty
[@sinsoku]: https://github.com/sinsoku
[@pocke]: https://github.com/pocke
9 changes: 9 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,15 @@ Rails/Present:
# Convert usages of `unless blank?` to `if present?`
UnlessBlank: true

Rails/RakeEnvironment:
Description: 'Set `:environment` task as a dependency to all rake task.'
Enabled: true
Safe: false
VersionAdded: '2.4'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a problem in most cases, but it is likely that calling :environment task will break a behavior. It may be better to mark it as Safe: false. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. I added Safe: false. Thanks!

Include:
- '**/Rakefile'
- '**/*.rake'

Rails/ReadWriteAttribute:
Description: >-
Checks for read_attribute(:attr) and
Expand Down
91 changes: 91 additions & 0 deletions lib/rubocop/cop/rails/rake_environment.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# This cop checks rake task definition without `environment` dependency.
# `environment` dependency is important because it loads application code
# for the rake task. The rake task cannot use application code, such as
# models, without `environment` dependency.
#
# You can ignore the offense if the task satisfies at least one of the
# following conditions:
#
# * The task does not need application code.
# * The task invokes :environment task.
#
# @example
# # bad
# task :foo do
# do_something
# end
#
# # good
# task foo: :environment do
# do_something
# end
#
class RakeEnvironment < Cop
MSG = 'Set `:environment` task as a dependency to all rake task.'

def_node_matcher :task_definition?, <<-PATTERN
(send nil? :task ...)
PATTERN

def on_send(node)
return unless task_definition?(node)
return if task_name(node) == :default
return if with_dependencies?(node)

add_offense(node)
end

private

def task_name(node)
first_arg = node.arguments[0]
case first_arg&.type
when :sym, :str
return first_arg.value.to_sym
when :hash
return nil if first_arg.children.size != 1

pair = first_arg.children.first
key = pair.children.first
case key.type
when :sym, :str
key.value.to_sym
end
end
end

def with_dependencies?(node)
first_arg = node.arguments[0]
return false unless first_arg

if first_arg.hash_type?
with_hash_style_dependencies?(first_arg)
else
task_args = node.arguments[1]
return false unless task_args
return false unless task_args.hash_type?

with_hash_style_dependencies?(task_args)
end
end

def with_hash_style_dependencies?(hash_node)
deps = hash_node.pairs.first&.value
return false unless deps

case deps.type
when :array
!deps.values.empty?
else
true
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
require_relative 'rails/pluralization_grammar'
require_relative 'rails/presence'
require_relative 'rails/present'
require_relative 'rails/rake_environment'
require_relative 'rails/read_write_attribute'
require_relative 'rails/redundant_allow_nil'
require_relative 'rails/redundant_receiver_in_with_options'
Expand Down
1 change: 1 addition & 0 deletions manual/cops.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
* [Rails/PluralizationGrammar](cops_rails.md#railspluralizationgrammar)
* [Rails/Presence](cops_rails.md#railspresence)
* [Rails/Present](cops_rails.md#railspresent)
* [Rails/RakeEnvironment](cops_rails.md#railsrakeenvironment)
* [Rails/ReadWriteAttribute](cops_rails.md#railsreadwriteattribute)
* [Rails/RedundantAllowNil](cops_rails.md#railsredundantallownil)
* [Rails/RedundantReceiverInWithOptions](cops_rails.md#railsredundantreceiverinwithoptions)
Expand Down
37 changes: 37 additions & 0 deletions manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,43 @@ NotNilAndNotEmpty | `true` | Boolean
NotBlank | `true` | Boolean
UnlessBlank | `true` | Boolean

## Rails/RakeEnvironment

Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
--- | --- | --- | --- | ---
Enabled | No | No | 2.4 | -

This cop checks rake task definition without `environment` dependency.
`environment` dependency is important because it loads application code
for the rake task. The rake task cannot use application code, such as
models, without `environment` dependency.

You can ignore the offense if the task satisfies at least one of the
following conditions:

* The task does not need application code.
* The task invokes :environment task.

### Examples

```ruby
# bad
task :foo do
do_something
end

# good
task foo: :environment do
do_something
end
```

### Configurable attributes

Name | Default value | Configurable values
--- | --- | ---
Include | `**/Rakefile`, `**/*.rake` | Array

## Rails/ReadWriteAttribute

Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
Expand Down
86 changes: 86 additions & 0 deletions spec/rubocop/cop/rails/rake_environment_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::RakeEnvironment do
subject(:cop) { described_class.new(config) }

let(:config) { RuboCop::Config.new }

it 'registers an offense to task without :environment' do
expect_offense(<<~RUBY)
task :foo do
^^^^^^^^^ Set `:environment` task as a dependency to all rake task.
end
RUBY
end

it 'does not register an offense to task with :environment ' \
'but it has other dependency before it' do
expect_no_offenses(<<~RUBY)
task foo: [:bar, `:environment`] do
end
RUBY
end

it 'does not register an offense to task with an dependency' do
expect_no_offenses(<<~RUBY)
task foo: :bar do
end
RUBY
end

it 'does not register an offense to task with dependencies' do
expect_no_offenses(<<~RUBY)
task foo: [:foo, :bar] do
end
RUBY
end

it 'does not register an offense to task with a dependency ' \
'as a method call' do
expect_no_offenses(<<~RUBY)
task foo: [:bar, dep]
RUBY
end

it 'does not register an offense to task with :environment' do
expect_no_offenses(<<~RUBY)
task foo: `:environment` do
end
RUBY
end

it 'does not register an offense to task with :environment ' \
'and other dependencies' do
expect_no_offenses(<<~RUBY)
task foo: [`:environment`, :bar] do
end
RUBY
end

it 'does not register an offense to task with :environment and an argument' do
expect_no_offenses(<<~RUBY)
task :foo, [:arg] => `:environment` do
end
RUBY
end

it 'does not register an offense to task with a dependency ' \
'as a method call' do
expect_no_offenses(<<~RUBY)
task foo: dep
RUBY
end

it 'does not register an offense to task with dependencies ' \
'as a method call' do
expect_no_offenses(<<~RUBY)
task foo: [dep, :bar]
RUBY
end

it 'does not register an offense to the default task' do
expect_no_offenses(<<~RUBY)
task default: :spec
RUBY
end
end