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

# Address false positive when reversing remove_foreign_key #205

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

joshpencheon
Copy link
Contributor

@joshpencheon joshpencheon commented Feb 20, 2020

The other table can be specified either as the second argument, or in the :to_table key of a hash as the second argument. The latter was not supported.

# was bad, now good:
def change
  remove_foreign_key :accounts, to_table: :branches
end

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).
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@joshpencheon joshpencheon force-pushed the remove_foreign_key_to_table branch 2 times, most recently from dff35b8 to 52b1e92 Compare February 20, 2020 17:11
@@ -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.

@@ -169,6 +169,11 @@ def change
remove_foreign_key :accounts, :branches
RUBY

it_behaves_like 'accepts',
'remove_foreign_key(with to_table option)', <<-RUBY
Copy link
Member

@koic koic Feb 21, 2020

Choose a reason for hiding this comment

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

Can you update with squiggly heredoc?

Suggested change
'remove_foreign_key(with to_table option)', <<-RUBY
'remove_foreign_key(with to_table option)', <<~RUBY

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 found with squiggly heredoc another style rule complained about indentation (see other >80 char workarounds earlier in the file), but have shortened spec label and it fits now.

@@ -169,6 +169,10 @@ def change
remove_foreign_key :accounts, :branches
RUBY

it_behaves_like 'accepts', 'remove_foreign_key(with table option)', <<~RUBY
Copy link
Member

Choose a reason for hiding this comment

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

Would you update with the description using :to_table? This suggestion is less than 80 characters :-)

Suggested change
it_behaves_like 'accepts', 'remove_foreign_key(with table option)', <<~RUBY
it_behaves_like 'accepts', 'remove_foreign_key(with `:to_table`)', <<~RUBY

The other table can be specified either as the second argument, or in
the :to_table key of a hash as the second argument. The latter was not
supported.
@koic koic merged commit 7506a31 into rubocop:master Feb 21, 2020
@koic
Copy link
Member

koic commented Feb 21, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants