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

Add Rails/AddColumnIndex cop to prevent passing an unused but confusing index: key to add_column migrations #497

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented May 25, 2021

We ran into this at work. add_column in a migration seems to accept an index key like change_column or t.column syntax, but it does not actually do anything. This cop is to prevent appearing like an index will be added without actually adding one, ie.

add_column :table, :my_column, :string, index: true

If this is detected, the cop will register an offense, and correct by adding a add_index line following.


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

@koic
Copy link
Member

koic commented Jun 7, 2021

It would be better if it could this (and other) illegal option be detected at the migration runtime as well. Please ask the Rails team if you like :-) Anyway, this PR looks good to me. Can you rebase with the latest mater branch?

@koic
Copy link
Member

koic commented Jun 15, 2021

@dvandersluis ping :-)

…sing `index:` key to `add_column` migrations.
@dvandersluis
Copy link
Member Author

Sorry for the delay @koic, it's been rebased now!

There's rails/rails#39230 but it hasn't had any progress.

@koic
Copy link
Member

koic commented Jun 15, 2021

I get it! I didn't know the Rails's issue. Thank you!

@owst
Copy link

owst commented Oct 12, 2023

Just to note that it looks like this PR to Rails is included in Rails 7.1 rails/rails#46178 which I think will reject the invalid index option

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.

3 participants