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

find_by_* cop #3455

Closed
dkniffin opened this Issue Aug 30, 2016 · 9 comments

Comments

Projects
None yet
5 participants
@dkniffin
Contributor

dkniffin commented Aug 30, 2016

I'd like to suggest a new cop, which will look for instances of the old rails dynamic finders (ie: find_by_id) and prefer the non-dynamic version: (find_by(id: ...))

Edit: whoever implements this, be careful of instances such as find_by_sql, which is a legit method. Not sure if there are others.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 6, 2016

Collaborator

Sounds reasonable to me.

Collaborator

bbatsov commented Sep 6, 2016

Sounds reasonable to me.

@pocke

This comment has been minimized.

Show comment
Hide comment
@pocke

pocke Oct 8, 2016

Member

It's nice cop. 👍 I'll implement this.

Member

pocke commented Oct 8, 2016

It's nice cop. 👍 I'll implement this.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

@pocke Great!

Collaborator

bbatsov commented Oct 8, 2016

@pocke Great!

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```

@pocke pocke referenced this issue Oct 8, 2016

Merged

[Fix #3455] Add new `Rails/DynamicFindBy` cop #3588

10 of 10 tasks complete

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```

pocke added a commit to pocke/rubocop that referenced this issue Oct 8, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```

@bbatsov bbatsov closed this in #3588 Oct 8, 2016

bbatsov added a commit that referenced this issue Oct 8, 2016

[Fix #3455] Add new `Rails/DynamicFindBy` cop
Resolve #3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```
@nbulaj

This comment has been minimized.

Show comment
Hide comment
@nbulaj

nbulaj Oct 12, 2016

What about the situations when somebody adds a method like:

def self.find_by_name(value)
  where("json_column -> 'key1' ->> 'key2' = ?", value).first
end

This is not a deprecated dynamic finder method and it could not be rewritten using default Rails find_by syntax

nbulaj commented Oct 12, 2016

What about the situations when somebody adds a method like:

def self.find_by_name(value)
  where("json_column -> 'key1' ->> 'key2' = ?", value).first
end

This is not a deprecated dynamic finder method and it could not be rewritten using default Rails find_by syntax

@dkniffin

This comment has been minimized.

Show comment
Hide comment
@dkniffin

dkniffin Oct 12, 2016

Contributor

@nbulaj I think that's a rare (although definitely legit) case, and I'd use rubocop:disable Rails/DynamicFindBy above that.

Contributor

dkniffin commented Oct 12, 2016

@nbulaj I think that's a rare (although definitely legit) case, and I'd use rubocop:disable Rails/DynamicFindBy above that.

@pocke

This comment has been minimized.

Show comment
Hide comment
@pocke

pocke Oct 12, 2016

Member

@nbulaj
We can take two steps to avoid the problem.
First, @dkniffin says, disable the cop by inline comment.

Second, add find_by_name to whitelist.
https://github.com/bbatsov/rubocop/blob/6a3442b/config/default.yml#L1239-L1241

# In .rubocop.yml
Rails/DynamicFindBy:
  Whitelist:
    - find_by_sql
    - find_by_name
Member

pocke commented Oct 12, 2016

@nbulaj
We can take two steps to avoid the problem.
First, @dkniffin says, disable the cop by inline comment.

Second, add find_by_name to whitelist.
https://github.com/bbatsov/rubocop/blob/6a3442b/config/default.yml#L1239-L1241

# In .rubocop.yml
Rails/DynamicFindBy:
  Whitelist:
    - find_by_sql
    - find_by_name
@nbulaj

This comment has been minimized.

Show comment
Hide comment
@nbulaj

nbulaj Oct 12, 2016

@pocke is there are a few methods like that - yes, it will solve the problem. Time will tell :)

nbulaj commented Oct 12, 2016

@pocke is there are a few methods like that - yes, it will solve the problem. Time will tell :)

Neodelf added a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016

[Fix rubocop-hq#3455] Add new `Rails/DynamicFindBy` cop
Resolve rubocop-hq#3455

Feature
----

This cop checks dynamic `find_by_*` method.

```ruby

User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

User.find_by(email: email)
User.find_by!(name: name)
User.find_by(email: email, name: name)
```

Note
-----

When arguments size and column name size are different,
the cop registers an offence.
However, doesn't auto-correct.

e.g.

```ruby
User.find_by_name_and_email(name)
```
@paulodeon

This comment has been minimized.

Show comment
Hide comment
@paulodeon

paulodeon Jan 10, 2018

I believe this cop is not correct.

find_by_column_name dynamic finders are still ok

http://guides.rubyonrails.org/active_record_querying.html#dynamic-finders

paulodeon commented Jan 10, 2018

I believe this cop is not correct.

find_by_column_name dynamic finders are still ok

http://guides.rubyonrails.org/active_record_querying.html#dynamic-finders

@pocke

This comment has been minimized.

Show comment
Hide comment
@pocke

pocke Jan 10, 2018

Member

@paulodeon This cop does not say "dynamic finders don't work". I think it is a style cop. It is not a lint cop. Dynamic finders works well, but this cop enforces find_by(name: name) style.

Member

pocke commented Jan 10, 2018

@paulodeon This cop does not say "dynamic finders don't work". I think it is a style cop. It is not a lint cop. Dynamic finders works well, but this cop enforces find_by(name: name) style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment