Add new Rails/MigrationTimestamp cop

[sambostock]

This cop enforces that migration file names start with a valid timestamp.

It checks both for the correct timestamp format (14 digits), as well as actual timestamp validity (YYYYmmddHHMMSS), by parsing with Time.strptime, and checking if Time#strftime returns the original timestamp, to ensure normalization of exotic dates like February 31st (i.e. March 2nd, depending on leap years).

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• 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.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[sambostock]

I noticed that MigrationClassName opts to include everything under db, and uses a node pattern to detect if the file looks like a migration. We could probably do something similar here, if that's preferable.

[davidstosik]

if the file looks like a migration

I've wondered about that.
What does "look like a migration" mean really? (We can't rely on the filename having a timestamp prefix, considering that's what we want to check here.)
Would you open the migration file and check if it defines a class that inherits from ActiveRecord::Migration? What if a project implements its own ApplicationMigration base class?

[sambostock]

Would you open the migration file and check if it defines a class that inherits from ActiveRecord::Migration?

That is exactly what it does! 😅

[davidstosik]

Now I think about it again, I think my suggestion to split at the first _ would work, since you check time.strftime(format) == timestamp:

timestamp = File.basename(file_path).split("_", 2).first
time = Time.strptime(timestamp, format) && time.strftime(format) == timestamp

I'm happy with the regular expression anyway!

Thanks a lot for writing this! ❤️

[davidstosik]

I was about to suggest splitting at the first _ for a more readable version, but it turns out Time.strptime will accept more characters than the pattern needs, and non-numeric characters too...

# Proper time
Time.strptime("20230505163015", "%Y%m%d%H%M%S")
#=> 2023-05-05 16:30:15 +0900

# Extra character
Time.strptime("20230505163015a", "%Y%m%d%H%M%S")
#=> 2023-05-05 16:30:15 +0900

# Non-numeric character
irb(main):008:0> Time.strptime("2023050516301a", "%Y%m%d%H%M%S")
=> 2023-05-05 16:30:01 +0900

Regex looks good! 👍🏻

[davidstosik]

if the file looks like a migration

I've wondered about that.
What does "look like a migration" mean really? (We can't rely on the filename having a timestamp prefix, considering that's what we want to check here.)
Would you open the migration file and check if it defines a class that inherits from ActiveRecord::Migration? What if a project implements its own ApplicationMigration base class?

[davidstosik]

Smart! 💯

[sambostock]

@davidstosik I ended up switching to the split approach. Since we're parsing and round tripping, we implicitly get the length check, so we can go with the more readable option.

@paracycle I've added the a check against future timestamps.

[sambostock]

@koic If you have time, this is ready for review.