-
Notifications
You must be signed in to change notification settings - Fork 0
Tech 18196 fix bug in value validations and expand functionality of timestamps dsl #182
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
Conversation
| def timestamps(**options) | ||
| field(:created_at, :datetime, null: false, **options) | ||
| field(:updated_at, :datetime, null: false, **options) | ||
| 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.
Is this a breaking change? Will this cause all created_at and updated_at fields to need migrations?
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 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.
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 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:
- Minor version bump: Add new functionality of passing options to timestamp fields.
- Major version bump: Change default value of
null: truetonull: false
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.
If you don't think there are other users of our gem, I'm fine with doing this as a minor version bump
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 a good point. Dependents graph says only 4 other users, so minor seems good for this
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.
But, I agree we should follow best practice here. I'll cut it as major. fd8668e
ttstarck
left a comment
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.
Just one final fix for a typo in the changelog
Co-authored-by: Tristan Starck <ttstarck@gmail.com>
[2.3.3] - Unreleased
Changed
timestampsDSL method to createcreated_atandupdated_atcolumns now defaults tonull: falsefordatetimecolumnstimestampsDSL method to allow additional options to be passed to thedatetimefieldsFixed
#validatemethods on core object classes with required arguments were cuasing model validations to fail