Skip to content

Conversation

@jebentier
Copy link
Contributor

[2.3.3] - Unreleased

Changed

  • The timestamps DSL method to create created_at and updated_at columns now defaults to null: false for datetime columns
  • The timestamps DSL method to allow additional options to be passed to the datetime fields

Fixed

  • Fixed a bug where #validate methods on core object classes with required arguments were cuasing model validations to fail

@jebentier jebentier marked this pull request as ready for review April 8, 2025 14:41
@jebentier jebentier requested a review from a team as a code owner April 8, 2025 14:41
Comment on lines +20 to 23
def timestamps(**options)
field(:created_at, :datetime, null: false, **options)
field(:updated_at, :datetime, null: false, **options)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a breaking change? Will this cause all created_at and updated_at fields to need migrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 It could be considered breaking, but there is the ability to have previous functionality by using timestamps null: true to override the new default. I think that's enough to not require the breaking change bump, but not strongly opposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be considered breaking but there is the ability to have previous functionality by using timestamps null: true to override the new default

This is definitely breaking since it requires code changes, its just good practice to provide an easy way of going back to old behavior 😅. This should also be a minor version at the minimum since this is adding new functionality too.

I don't know if other people are really using this gem besides invoca but if there are, we should definitely make this a breaking change.

Or we can split this into two PRs:

  1. Minor version bump: Add new functionality of passing options to timestamp fields.
  2. Major version bump: Change default value of null: true to null: false

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't think there are other users of our gem, I'm fine with doing this as a minor version bump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Dependents graph says only 4 other users, so minor seems good for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, I agree we should follow best practice here. I'll cut it as major. fd8668e

@jebentier jebentier requested a review from ttstarck April 8, 2025 17:16
Copy link
Contributor

@ttstarck ttstarck left a comment

Choose a reason for hiding this comment

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

Just one final fix for a typo in the changelog

Co-authored-by: Tristan Starck <ttstarck@gmail.com>
@jebentier jebentier merged commit acc9c67 into master Apr 8, 2025
42 checks passed
@jebentier jebentier deleted the TECH-18196_patch_datetime_timestamps branch April 8, 2025 20:12
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