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.

Sign up for GitHub

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

Jump to bottom

# Address false positive when reversing remove_foreign_key #205

Merged
merged 1 commit into from
Feb 21, 2020
Merged

# Address false positive when reversing remove_foreign_key #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

* [#180](https://github.com/rubocop-hq/rubocop-rails/issues/180): Fix a false positive for `HttpPositionalArguments` when using `get` method with `:to` option. ([@koic][])
* [#193](https://github.com/rubocop-hq/rubocop-rails/issues/193): Make `Rails/EnvironmentComparison` aware of `Rails.env` is used in RHS or when `!=` is used for comparison. ([@koic][])
* [#205](https://github.com/rubocop-hq/rubocop-rails/pull/205): Make `Rails/ReversibleMigration` aware of `:to_table` option of `remove_foreign_key`. ([@joshpencheon][])
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the following failure test?

Failures:

  1) RuboCop Rails Project changelog has link definitions for all implicit links
     Failure/Error:
       expect(changelog.include?("[#{name}]: http"))
         .to be(true), "CHANGELOG.md is missing a link for #{name}. " \
                       'Please add this link to the bottom of the file.'
     
       CHANGELOG.md is missing a link for @joshpencheon. Please add this link to the bottom of the file.
     # ./spec/project_spec.rb:27:in `block (4 levels) in <top (required)>'
     # ./spec/project_spec.rb:26:in `each'
     # ./spec/project_spec.rb:26:in `block (3 levels) in <top (required)>'

Finished in 1.85 seconds (files took 1.32 seconds to load)
2239 examples, 1 failure

https://circleci.com/gh/rubocop-hq/rubocop-rails/2700

You can see also CONTRIBUTING.md:

If this is your first contribution to RuboCop project, add a link definition for the implicit link to the bottom of the changelog as [@username]: https://github.com/username.

https://circleci.com/gh/rubocop-hq/rubocop-rails/2700

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done thanks - sorry, the local suite didn't have that failure.


## 2.4.2 (2020-01-26)

Expand Down Expand Up @@ -141,3 +142,4 @@
[@mvz]: https://github.com/mvz
[@fidalgo]: https://github.com/fidalgo
[@hanachin]: https://github.com/hanachin
[@joshpencheon]: https://github.com/joshpencheon
7 changes: 6 additions & 1 deletion lib/rubocop/cop/rails/reversible_migration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ module Rails
# remove_foreign_key :accounts, :branches
# end
#
# # good
# def change
# remove_foreign_key :accounts, to_table: :branches
# end
#
# @example
# # change_table
#
Expand Down Expand Up @@ -210,7 +215,7 @@ def check_remove_column_node(node)

def check_remove_foreign_key_node(node)
remove_foreign_key_call(node) do |arg|
if arg.hash_type?
if arg.hash_type? && !all_hash_key?(arg, :to_table)
add_offense(
node,
message: format(MSG,
Expand Down
5 changes: 5 additions & 0 deletions manual/cops_rails.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2053,6 +2053,11 @@ end
def change
remove_foreign_key :accounts, :branches
end

# good
def change
remove_foreign_key :accounts, to_table: :branches
end
```
```ruby
# change_table
Expand Down
4 changes: 4 additions & 0 deletions spec/rubocop/cop/rails/reversible_migration_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,10 @@ def change
remove_foreign_key :accounts, :branches
RUBY

it_behaves_like 'accepts', 'remove_foreign_key(with :to_table)', <<~RUBY
remove_foreign_key :accounts, to_table: :branches
RUBY

it_behaves_like 'offense', 'remove_foreign_key(without table)', <<~RUBY
remove_foreign_key :accounts, column: :owner_id
RUBY
Expand Down