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

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

Merged
merged 1 commit into from Oct 8, 2016

Conversation

Projects
None yet
2 participants
@pocke
Member

pocke commented Oct 8, 2016

Resolve #3455

Feature

This cop checks dynamic find_by_* method.

# Bad code
User.find_by_email(email)
User.find_by_name!(name)
User.find_by_email_and_name(email, name)

# Good code
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.

# It's a bad code, howver, can't auto correct.
User.find_by_name_and_email(name)

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
Show outdated Hide outdated config/enabled.yml Outdated
Show outdated Hide outdated config/enabled.yml Outdated
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

Apart from my tiny remark, the new cop looks good.

Collaborator

bbatsov commented Oct 8, 2016

Apart from my tiny remark, the new cop looks good.

# Returns static method name.
# If code isn't wrong, returns nil
def static_method_name(method_name)
match = METHOD_PATTERN.match(method_name)

This comment has been minimized.

@bbatsov

bbatsov Oct 8, 2016

Collaborator

This reminded me it'd be nice to have a cop enforcing consistent use of =~ or match. :-)

@bbatsov

bbatsov Oct 8, 2016

Collaborator

This reminded me it'd be nice to have a cop enforcing consistent use of =~ or match. :-)

@pocke pocke referenced this pull request Oct 8, 2016

Merged

Add a test to check cop message style #3589

8 of 10 tasks complete
@pocke

This comment has been minimized.

Show comment
Hide comment
@pocke

pocke Oct 8, 2016

Member

Fixed.
Thanks for the review.

Member

pocke commented Oct 8, 2016

Fixed.
Thanks for the review.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

You'll also have to rebase on top of the current master.

Collaborator

bbatsov commented Oct 8, 2016

You'll also have to rebase on top of the current master.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

I hate our changelog... Another merge conflict there...

Collaborator

bbatsov commented Oct 8, 2016

I hate our changelog... Another merge conflict there...

[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)
```
@pocke

This comment has been minimized.

Show comment
Hide comment
@pocke

pocke Oct 8, 2016

Member

np. rebased.

We need better management way of CHANGELOG. But I don't have any idea 😢

Member

pocke commented Oct 8, 2016

np. rebased.

We need better management way of CHANGELOG. But I don't have any idea 😢

@bbatsov bbatsov merged commit 2d71e03 into rubocop-hq:master Oct 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pocke pocke deleted the pocke:dynamic-find-by-cop branch Oct 8, 2016

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Oct 8, 2016

Collaborator

We need better management way of CHANGELOG. But I don't have any idea 😢

It's a tough nut to crack - have been wondering about this for a while. Some people generate the changelog from the commit messages, but that's also problematic.

Collaborator

bbatsov commented Oct 8, 2016

We need better management way of CHANGELOG. But I don't have any idea 😢

It's a tough nut to crack - have been wondering about this for a while. Some people generate the changelog from the commit messages, but that's also problematic.

pocke added a commit to pocke/rubocop that referenced this pull request Oct 10, 2016

Change merge driver for CHANGELOG.md to resolve conflict problem
Currently Problem
------

CHANGELOG.md is very often conflict when PR.
It's very troublesome.
e.g. rubocop-hq#3588 (comment)

Solution
-----

I've added a `.gitattributes` file.
The file changes a merge driver to union for CHANGELOG.md.

The union driver has what it takes to merge CHANGELOG.md.
With the setting, CHANGELOG doesn't conflict!
Always both of the addition is chosen.

However, this solution has a problem.
GitHub doesn't support the feature.
So, We can't merge a PR at GitHub Web if CHANGELOG.md conflicts.

Don't worry, `hub` command supports the feature.
Maintainer can merge a conflicted PR by hub command.
For example.

```sh
$ git branch
* master
some-feature-branch-1

$ hub merge https://github.com/bbatsov/rubocop/pull/xxxx # specify a PR url
$ git push
```

I think this method is not complete, but it is better than now.
What do you think?

More Information
-----

- http://krlmlr.github.io/using-gitattributes-to-avoid-merge-conflicts/

bbatsov added a commit that referenced this pull request Oct 11, 2016

Change merge driver for CHANGELOG.md to resolve conflict problem
Currently Problem
------

CHANGELOG.md is very often conflict when PR.
It's very troublesome.
e.g. #3588 (comment)

Solution
-----

I've added a `.gitattributes` file.
The file changes a merge driver to union for CHANGELOG.md.

The union driver has what it takes to merge CHANGELOG.md.
With the setting, CHANGELOG doesn't conflict!
Always both of the addition is chosen.

However, this solution has a problem.
GitHub doesn't support the feature.
So, We can't merge a PR at GitHub Web if CHANGELOG.md conflicts.

Don't worry, `hub` command supports the feature.
Maintainer can merge a conflicted PR by hub command.
For example.

```sh
$ git branch
* master
some-feature-branch-1

$ hub merge https://github.com/bbatsov/rubocop/pull/xxxx # specify a PR url
$ git push
```

I think this method is not complete, but it is better than now.
What do you think?

More Information
-----

- http://krlmlr.github.io/using-gitattributes-to-avoid-merge-conflicts/

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

Change merge driver for CHANGELOG.md to resolve conflict problem
Currently Problem
------

CHANGELOG.md is very often conflict when PR.
It's very troublesome.
e.g. rubocop-hq#3588 (comment)

Solution
-----

I've added a `.gitattributes` file.
The file changes a merge driver to union for CHANGELOG.md.

The union driver has what it takes to merge CHANGELOG.md.
With the setting, CHANGELOG doesn't conflict!
Always both of the addition is chosen.

However, this solution has a problem.
GitHub doesn't support the feature.
So, We can't merge a PR at GitHub Web if CHANGELOG.md conflicts.

Don't worry, `hub` command supports the feature.
Maintainer can merge a conflicted PR by hub command.
For example.

```sh
$ git branch
* master
some-feature-branch-1

$ hub merge https://github.com/bbatsov/rubocop/pull/xxxx # specify a PR url
$ git push
```

I think this method is not complete, but it is better than now.
What do you think?

More Information
-----

- http://krlmlr.github.io/using-gitattributes-to-avoid-merge-conflicts/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment