-
-
Notifications
You must be signed in to change notification settings - Fork 263
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 new MigrationClassName
cop
#644
Conversation
167304e
to
34b9016
Compare
private | ||
|
||
def to_snakecase(word) | ||
word.gsub!(/([A-Z\d]+)([A-Z][a-z])/, '\1_\2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this shouldn't mutate the word, i.e.:
word
.gsub(/([A-Z\d]+)([A-Z][a-z])/, '\1_\2')
.gsub(/([a-z\d])([A-Z])/, '\1_\2')
.tr('-', '_')
.downcase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. Thanks!
def on_new_investigation | ||
filepath = processed_source.file_path | ||
@basename = File.basename(filepath, '.rb') | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these logic in the on_class
method and refactor to prevent the use of the instance variable @basename
?
https://refactoring.com/catalog/inlineFunction.html
def on_class(node) | ||
class_name = node.loc.name.source | ||
snake_class_name = to_snakecase(class_name) | ||
basename_without_timestamp = @basename.sub(/^[0-9]+_/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basename_without_timestamp = @basename.sub(/^[0-9]+_/, '') | |
basename_without_timestamp = @basename.delete_prefix(/\A\d+_/, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! It was correctly as follows.
- basename_without_timestamp = @basename.delete_prefix(/\A\d+_/, '')
+ basename_without_timestamp = @basename.sub(/\A\d+_/, '')
it do | ||
expect_offense(<<~RUBY, filename) | ||
class SellBooks < ActiveRecord::Migration[7.0] | ||
^^^^^^^^^ The class name does not match with the file name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be user-friendly to indicate the class name to replace.
^^^^^^^^^ The class name does not match with the file name. | |
^^^^^^^^^ Replace with `CreateUsers` that matches the file name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think this cop can provide autocorrect the class name.
# class CreateUsers < ActiveRecord::Migration[7.0] | ||
# end | ||
# | ||
class MigrationFileName < Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MigrationClassName
seems to be more suitable than MigrationFileName
.
class MigrationFileName < Base | |
class MigrationClassName < Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's absolutely right! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the title of this pull request as well.
MigrationFileName
copMigrationClassName
cop
class MigrationClassName < Base | ||
extend AutoCorrector | ||
|
||
MSG = 'Replace with `%<preferred_class_name>s` that matches the file name.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "corrected" is better than "preferred" . Because it ’s not a difference in style :-)
MSG = 'Replace with `%<preferred_class_name>s` that matches the file name.' | |
MSG = 'Replace with `%<corrected_class_name>s` that matches the file name.' |
preferred_class_name = to_camelcase(basename_without_timestamp) | ||
message = format(MSG, preferred_class_name: preferred_class_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preferred_class_name = to_camelcase(basename_without_timestamp) | |
message = format(MSG, preferred_class_name: preferred_class_name) | |
corrected_class_name = to_camelcase(basename_without_timestamp) | |
message = format(MSG, corrected_class_name: corrected_class_name) |
add_offense(node.loc.name, message: message) do |corrector| | ||
corrector.replace(node.loc.name, preferred_class_name) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_offense(node.loc.name, message: message) do |corrector| | |
corrector.replace(node.loc.name, preferred_class_name) | |
end | |
add_offense(node.identifier, message: message) do |corrector| | |
corrector.replace(node.identifier, preferred_class_name) | |
end |
Nice! Can you squash your commits into one? |
16787cf
to
705c794
Compare
Thanks for reviewing! By the way, is there something like a wiki for writing better cops? I read source code in rubocop-ast repository to learn what |
Here is an introductory document, but I'm also learning by reading the source code. |
This cop makes sure that each migration file defines a migration class whose name matches the file name.
(i.e.
20220224111111_create_users.rb
should defineCreateUsers
class.)The migration will fail when you rename the migration class and forget to rename the file name appropriately. Unfortunately, it's hard to detect this kind of bug with automated tests that use database tables restored from db/schema.rb (e.g.
db:setup
). The static inspection for the file names could help to prevent the bug.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.and description in grammatically correct, complete sentences.