-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 Rails/CreateTableWithTimestamps
cop
#5077
Add new Rails/CreateTableWithTimestamps
cop
#5077
Conversation
544d810
to
848d34c
Compare
This seems like a good idea. Here are some questions to consider:
I'm ok with answering "no" to all of those, just asking ... |
|
||
def on_send(node) | ||
return unless create_table_call?(node) | ||
ancestor = node.ancestors.first |
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 we can use node.parent
instead of ancestors.first
.
(block | ||
(send nil? :create_table ...) | ||
(args (arg _var)) | ||
begin) |
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.
The last begin
matcher is not correct. This pattern does not match empty blocks or blocks that have one statement only.
For example, this cop should add offenses for the following code, but it does not add:
create_table :users do |t|
end
create_table :users do |t|
t.string :name
end
Because the ASTs for this cases don't have begin
node.
$ ruby-parse test.rb
(begin
(block
(send nil :create_table
(sym :users))
(args
(procarg0 :t)) nil)
(block
(send nil :create_table
(sym :users))
(args
(procarg0 :t))
(send
(lvar :t) :string
(sym :name))))
ancestor = node.ancestors.first | ||
|
||
if create_table_with_block?(ancestor) | ||
unless ancestor.body.child_nodes.any? { |child| timestamps?(child) } |
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.
We can use def_node_search
instead of child_nodes.any?
.
For example:
begin) | ||
PATTERN | ||
|
||
def_node_matcher :create_table_call?, <<-PATTERN |
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.
This use of a node matcher is a bit gratuitous. You can just use:
node.command?(:create)
on send nodes anywhere. 🙂
@mikegee Thanks for your questions.
Yes. That is a good idea. I will fix
Yes. If they are declared manually, this cop should not add offenses. I will fix it.
Yes. If a user explicitly declared only one of them, I think we can judge that a user has not forgotten to add timestamps. I will also fix this. |
This cop checks the migration for which timestamps are not included when creating a new table. In many cases, timestamps are useful information and should be added.
2335163
to
f6e0936
Compare
Hi there,
The other day I made a mistake in forgetting to add timestamps when creating a new table :(
So, I want to prevent such simple mistakes by using RuboCop.
This cop checks the migration for which timestamps are not included when creating a new table. In many cases, timestamps are useful information and should be added.
WDYT?
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).rake spec
) are passing.rake internal_investigation
.and description in grammatically correct, complete sentences.
rake generate_cops_documentation
(required only when you've added a new cop or changed the configuration/documentation of an existing cop).