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

Register new offenses for #294

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

PhilCoggins
Copy link
Contributor

Adds missing offenses for remove_columns and remove_index methods.

This is my first Rubocop contribution, so please review carefully!


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.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.
  • 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.

# end
# end
#
# # good (Rails >= 6.1)
Copy link
Member

@koic koic Jul 20, 2020

Choose a reason for hiding this comment

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

Rails 6.1 has not been released yet. Can you provide a URL of rails/rails repo's PR that includes this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah just found it by chance in this comment.

rails/rails#36589

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koic Did you just want to see the PR or did you want me to actually add to the comment in code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@koic updated.

CHANGELOG.md Outdated Show resolved Hide resolved
@PhilCoggins PhilCoggins force-pushed the reversible_migration_cop_additions branch from 1dc1191 to a73cd5a Compare July 20, 2020 13:35
@PhilCoggins PhilCoggins requested a review from koic July 20, 2020 13:41
@PhilCoggins PhilCoggins force-pushed the reversible_migration_cop_additions branch 3 times, most recently from a157b3f to 3b3b4f5 Compare July 27, 2020 20:11
@koic
Copy link
Member

koic commented Jul 30, 2020

I added shared_context for Rails 6.1 to master branch.
159eef0

Can you rebase with the latest master branch and add spec for Rails 6.0 and Rails 6.1 context behaviours?
The following is an example:
https://github.com/rubocop-hq/rubocop-rails/blob/master/spec/rubocop/cop/rails/inverse_of_spec.rb#L91-L108

@PhilCoggins PhilCoggins force-pushed the reversible_migration_cop_additions branch 2 times, most recently from 304f073 to fa62b32 Compare July 30, 2020 02:03
@PhilCoggins
Copy link
Contributor Author

@koic done!

CHANGELOG.md Outdated
@@ -48,6 +48,7 @@
* [#263](https://github.com/rubocop-hq/rubocop-rails/pull/263): Change terminology to `ForbiddenMethods` and `AllowedMethods`. ([@jcoyne][])
* [#289](https://github.com/rubocop-hq/rubocop-rails/pull/289): Update `Rails/SkipsModelValidations` to register an offense for `insert_all`, `touch_all`, `upsert_all`, etc. ([@eugeneius][])
* [#293](https://github.com/rubocop-hq/rubocop-rails/pull/293): Require RuboCop 0.87 or higher. ([@koic][])
* [#294](https://github.com/rubocop-hq/rubocop-rails/pull/294): Update `Rails/ReversibleMigration` to register offenses for `remove_columns` and `remove_index`. ([@philcoggins][])
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 rebase with the latest master branch and move this entry to under the ### Changes of ## master (unreleased) section?


dir.down do
add_column :users, :name, :string
add_column :users, :name, :email
Copy link
Member

Choose a reason for hiding this comment

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

Is this a typo?

Suggested change
add_column :users, :name, :email
add_column :users, :email, :string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, but I can't regenerate docs now:

bundle exec rake default                        
rake aborted!
LoadError: cannot load such file -- rubocop/cops_documentation_generator
tasks/cops_documentation.rake:6:in `require'
tasks/cops_documentation.rake:6:in `<top (required)>'
/Users/philcoggins/Workbench/rubocop-rails/Rakefile:6:in `load'
/Users/philcoggins/Workbench/rubocop-rails/Rakefile:6:in `block in <top (required)>'
/Users/philcoggins/Workbench/rubocop-rails/Rakefile:6:in `each'
/Users/philcoggins/Workbench/rubocop-rails/Rakefile:6:in `<top (required)>'
/Users/philcoggins/.rbenv/versions/2.6.6/bin/bundle:23:in `load'
/Users/philcoggins/.rbenv/versions/2.6.6/bin/bundle:23:in `<main>'

Copy link
Member

Choose a reason for hiding this comment

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

Updated, but I can't regenerate docs now:

Can you try running bundle update?

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, the doc has been updated. I'm going to merge this PR. Thank you for your contribution!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I overlooked that the document generation hasn't finished yet 💦 I committed below:
d8f0f0b

@PhilCoggins PhilCoggins force-pushed the reversible_migration_cop_additions branch from fa62b32 to 1cfeea9 Compare July 31, 2020 14:32
@koic koic merged commit c6ccdca into rubocop:master Jul 31, 2020
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