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

Rails/HasManyOrHasOneDependent Cop & ActiveResource relations #6266

Closed
OtherCroissant opened this issue Sep 10, 2018 · 3 comments
Closed

Rails/HasManyOrHasOneDependent Cop & ActiveResource relations #6266

OtherCroissant opened this issue Sep 10, 2018 · 3 comments
Labels

Comments

@OtherCroissant
Copy link

Rails/HasManyOrHasOneDependent Cop wants me to define a :dependent option for a has_many relation for a class that extends from a ActiveResource model. Though, for a ActiveResource relation :dependent is not a valid option.


Expected behavior

The Rails/HasManyOrHasOneDependent error should not be triggered for ActiveResource models.

Actual behavior

Rails/HasManyOrHasOneDependent is detected

Steps to reproduce the problem

Include ActiveResource in your project:

gem 'activeresource', require: 'active_resource'

Create a ActiveResource class for talking to a REST API:

module API
  class User < ActiveResource::Base
    has_many :projects, class_name: 'API::Project'
  end
end

Running Rubocop gives the following:

app/models/api/user.rb:4:5: C: Rails/HasManyOrHasOneDependent: Specify a :dependent option.
    has_many :projects, class_name: 'API::Project'
    ^^^^^^^^

but If you add the :dependent option, ActiveResource will throw an exception on runtime:

Failure/Error: has_many :projects, class_name: 'API::Project', dependent: :destroy

ArgumentError:
  Unknown key: :dependent. Valid keys are: :class_name

RuboCop version

0.59.0 (using Parser 2.5.1.2, running on ruby 2.5.1 x86_64-darwin17)

@yskkin
Copy link

yskkin commented Sep 19, 2018

Does this mean that a module for ActiveRecord is no longer linted?

module Foo
  extend ActiveSupport::Concern

  included do
    has_many :bazs # 0.59.1 emits warning for this line
  end
end

class Bar < ApplicationRecord
  include Foo
end

@koic
Copy link
Member

koic commented Sep 21, 2018

@yskkin Thanks for the feedback. I think that it is better to check also when using mix-in.
I commented an idea on #6298 (comment).

The following is a quotation.

This is one idea. It seems better to check whether it is ActiveResource::Base instead of ActiveRecord::Base or ApplicationRecord. It does not check if the superclass is ActiveResource::Base.

What do you think about it?

Cc @tejasbubane @bbatsov

@koic
Copy link
Member

koic commented Sep 21, 2018

I opened a PR #6318 that implements the above idea.

koic added a commit to koic/rubocop that referenced this issue Sep 23, 2018
Follow up of
rubocop#6266 (comment).

This PR fixes a false negative for `Rails/HasManyOrHasOneDependent`
when using the following mix-in case.

```ruby
module Foo
  extend ActiveSupport::Concern

  included do
    has_many :bazs # 0.59.1 emits offense for this line
  end
end

class Bar < ApplicationRecord
  include Foo
end
```

In rubocop#6298, only subclasses of `AactiveRecord::Base` or `ApplicationRecord`
were scoped. But in that approach false negatives occur in the above case.
This PR changes the approach to solve the issue rubocop#6266 by skipping it
in case of subclass of `ActiveResource::Base`.

Also, this PR integrates a series of change log entry which has not yet
been released for the fixing of rubocop#6266.
bbatsov pushed a commit that referenced this issue Sep 24, 2018
Follow up of
#6266 (comment).

This PR fixes a false negative for `Rails/HasManyOrHasOneDependent`
when using the following mix-in case.

```ruby
module Foo
  extend ActiveSupport::Concern

  included do
    has_many :bazs # 0.59.1 emits offense for this line
  end
end

class Bar < ApplicationRecord
  include Foo
end
```

In #6298, only subclasses of `AactiveRecord::Base` or `ApplicationRecord`
were scoped. But in that approach false negatives occur in the above case.
This PR changes the approach to solve the issue #6266 by skipping it
in case of subclass of `ActiveResource::Base`.

Also, this PR integrates a series of change log entry which has not yet
been released for the fixing of #6266.
koic added a commit to rubocop/rubocop-rails that referenced this issue Oct 26, 2018
Follow up of
rubocop/rubocop#6266 (comment).

This PR fixes a false negative for `Rails/HasManyOrHasOneDependent`
when using the following mix-in case.

```ruby
module Foo
  extend ActiveSupport::Concern

  included do
    has_many :bazs # 0.59.1 emits offense for this line
  end
end

class Bar < ApplicationRecord
  include Foo
end
```

In #6298, only subclasses of `AactiveRecord::Base` or `ApplicationRecord`
were scoped. But in that approach false negatives occur in the above case.
This PR changes the approach to solve the issue #6266 by skipping it
in case of subclass of `ActiveResource::Base`.

Also, this PR integrates a series of change log entry which has not yet
been released for the fixing of #6266.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants