Add new `Rails/DateAndTimeColumnName` cop #335

fatkodima · 2020-08-23T00:26:00Z

Inspired by https://github.com/thoughtbot/guides/tree/master/rails

Name date columns with _on suffixes.
Name datetime columns with _at suffixes.
Name time columns (referring to a time of day with no date) with _time suffixes.

This enforces consistency and it is easier to tell the type from name without referring to db schema.

koic · 2020-08-23T00:37:00Z

There is reason why it is hard to apply by default. Updating db/migrate files that has already been migrated will result in schema mismatch.

koic · 2020-08-23T00:38:04Z

#277 also has the same problem.

fatkodima · 2020-08-23T00:39:51Z

We can add a config option (something like StartAfter: 20200907202002) from which version to start checking.

koic · 2020-08-23T00:56:19Z

Yeah, I also considered the timestamp value. Perhaps setting to .rubocop.yml is expensive to maintain. It may be better to use the timestamp value from db/schema.rb. But I have not yet fully considered false positives 😅

pirj · 2021-12-25T08:34:37Z

@fatkodima ping

fatkodima · 2021-12-25T08:59:25Z

@pirj

We can add a config option (something like StartAfter: 20200907202002) from which version to start checking.

So I need to implement this?

pirj · 2021-12-25T09:22:15Z

Another option that does not require configuration is inspecting db/*schema.rb files. Plural because there can many with multiple DB support.

pirj · 2021-12-25T09:27:46Z

Oh, that won't work, as schemas have all legacy columns. Yes, I believe the path taken in #277 is fine.

pirj

Looks good, thank you.
Some minor notes only.

PS Plus the StartAfterMigrationVersion options is needed.

pirj · 2021-12-25T10:05:16Z

config/default.yml

+  Enabled: pending
+  VersionAdded: '2.8'
+  Include:
+    - db/migrate/**/**.rb


Multiple db setup reads migrations_paths: from config/database.yml. I'm not sure if there's a convention on how to place them. Mine are usually in db/secondary_migrate/*.rb.

I changed it to

Include: - db/**/*.rb

like in #277
That way it will check all migration files (I hope no one places migration files using migrations_paths: into other folders than db/ 😄 )

config/default.yml

docs/modules/ROOT/pages/cops.adoc

pirj · 2021-12-25T10:14:45Z

lib/rubocop/cop/rails/date_and_time_column_name.rb

+        def_node_matcher :migration_base?, <<~PATTERN
+          {
+            (send (const (const nil? :ActiveRecord) :Migration) :[] _)
+            (const (const nil? :ActiveRecord) :Migration)


Wondering if some projects use a common ApplicationMigration 🤔
Is this check really necessary if we already know that we're inspecting db/migrations/*.rb?

With this change

Include: - db/**/*.rb

we should at least ignore schema.rb files. It is also possible (but very unlikely) to have some other files there, except migration files. Better to double check, I think.

lib/rubocop/cop/rails/date_and_time_column_name.rb

pirj · 2021-12-25T10:18:03Z

spec/rubocop/cop/rails/date_and_time_column_name_spec.rb

+  subject(:cop) { described_class.new }
+
+  context 'when `add_column` method' do
+    described_class::TYPE_TO_SUFFIX.each do |type, suffix|


This won't fail if you remove or add entries to TYPE_TO_SUFFIX. It also requires the reader of the spec to look it up in the implementation.

That is the way how similar things are tested in many places in the rubocop and rubocop-rails (for example,

rubocop-rails/spec/rubocop/cop/rails/validation_spec.rb

Line 28 in 647e46a

described_class::TYPES.each do |type|

). I was just stayed consistent with that style.

spec/rubocop/cop/rails/date_and_time_column_name_spec.rb

pirj · 2021-12-25T10:19:53Z

spec/rubocop/cop/rails/date_and_time_column_name_spec.rb

+    end
+  end
+
+  context 'when `<type>` method' do


Is this supposed to be interpolated with a real type?

That was meant method calls like t.integer ... or t.text ... etc. Changed it to t.<type> to be a little more clear.

koic · 2024-12-26T04:48:40Z

config/default.yml

+                `time` - with `_time` suffix.
+  Enabled: pending
+  VersionAdded: '<<next>>'
+  StartAfterMigrationVersion: null


Suggested change

StartAfterMigrationVersion: null

koic · 2024-12-26T04:48:42Z

lib/rubocop/cop/mixin/start_after_migration_version.rb

+      end
+    end
+  end
+end


#1383 was integrated. Can you remove this module?

koic · 2024-12-26T04:48:43Z

lib/rubocop/cop/rails/date_and_time_column_name.rb

+      #   t.column :visited_on, :date
+      #   t.time :start_time
+      #
+      # @example StartAfterMigrationVersion: 20211007000001


Can you remove this example?

koic · 2024-12-26T04:48:45Z

lib/rubocop/cop/rails/date_and_time_column_name.rb

+      #   add_column :orders, :created_on, :datetime
+      #
+      class DateAndTimeColumnName < Base
+        include StartAfterMigrationVersion


Suggested change

include StartAfterMigrationVersion

prepend MigrationsHelper

koic · 2024-12-26T04:48:47Z

lib/rubocop/cop/rails/date_and_time_column_name.rb

+        def in_migration?(node)
+          class_node = node.each_ancestor(:class).first
+          migration_base?(class_node&.parent_class)
+        end


This method can be removed and replaced with RuboCop::Cop::MigrationsHelper#in_migration?.

koic · 2024-12-26T04:49:36Z

This naming rule can probably be added to the Rails Style Guide.

fatkodima · 2024-12-26T09:58:57Z

Updated PR with the suggestions.

andyw8 mentioned this pull request Oct 4, 2020

Add new Rails/CalendarTypeSuffix cop #367

Closed

9 tasks

pirj approved these changes Dec 25, 2021

View reviewed changes

fatkodima force-pushed the date_and_time_column_name-cop branch from a880294 to d3ab782 Compare December 25, 2021 17:50

koic force-pushed the master branch from 45247fb to 6e31622 Compare January 9, 2022 20:57

fatkodima mentioned this pull request Apr 16, 2023

Config to ignore old migrations for Rails/ThreeStateBooleanColumn #985

Closed

9 tasks

Earlopain mentioned this pull request Oct 23, 2024

Add new Rails/IndexNames cop #1380

Open

9 tasks

koic reviewed Dec 26, 2024

View reviewed changes

lib/rubocop/cop/mixin/start_after_migration_version.rb Outdated

end

end

end

end

Copy link

Member

koic Dec 26, 2024

Choose a reason for hiding this comment

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

#1383 was integrated. Can you remove this module?

koic reviewed Dec 26, 2024

View reviewed changes

Add new Rails/DateAndTimeColumnName cop

d7f6c33

fatkodima force-pushed the date_and_time_column_name-cop branch from d3ab782 to d7f6c33 Compare December 26, 2024 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Rails/DateAndTimeColumnName` cop #335

Add new `Rails/DateAndTimeColumnName` cop #335

fatkodima commented Aug 23, 2020

koic commented Aug 23, 2020

koic commented Aug 23, 2020

fatkodima commented Aug 23, 2020

koic commented Aug 23, 2020

pirj commented Dec 25, 2021

fatkodima commented Dec 25, 2021

pirj commented Dec 25, 2021

pirj commented Dec 25, 2021

pirj left a comment •

edited

Loading

pirj Dec 25, 2021

fatkodima Dec 25, 2021

pirj Dec 25, 2021

fatkodima Dec 25, 2021

pirj Dec 25, 2021

fatkodima Dec 25, 2021

pirj Dec 25, 2021

fatkodima Dec 25, 2021

koic Dec 26, 2024

koic Dec 26, 2024

koic Dec 26, 2024

koic Dec 26, 2024

koic Dec 26, 2024

koic commented Dec 26, 2024

fatkodima commented Dec 26, 2024

Add new Rails/DateAndTimeColumnName cop #335

Are you sure you want to change the base?

Add new Rails/DateAndTimeColumnName cop #335

Conversation

fatkodima commented Aug 23, 2020

koic commented Aug 23, 2020

koic commented Aug 23, 2020

fatkodima commented Aug 23, 2020

koic commented Aug 23, 2020

pirj commented Dec 25, 2021

fatkodima commented Dec 25, 2021

pirj commented Dec 25, 2021

pirj commented Dec 25, 2021

pirj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Dec 26, 2024

fatkodima commented Dec 26, 2024

Add new `Rails/DateAndTimeColumnName` cop #335

Add new `Rails/DateAndTimeColumnName` cop #335

pirj left a comment •

edited

Loading