search

rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
968 stars 558 forks source link

Set precision 6 by default for timestamps #1057

Closed aidanharan closed 1 year ago

aidanharan commented 1 year ago

Timestamp columns should use precision 6 by default. This change was included in Rails 7.0.5 by https://github.com/rails/rails/pull/46110

The previous change to default datetime columns to use precision 6 was implemented in the adapter by https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/990

This fixes https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/issues/1056

aidanharan commented 1 year ago

@mgrunberg Could you review this please? If it's okay then I think you can close https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/pull/1058

mgrunberg commented 1 year ago

@aidanharan Sure, I'll review it later today/tomorrow.

mgrunberg commented 1 year ago

@aidanharan In general LGTM.

My only concern, which is not a blocker, is dumping existing datetime columns. When you have a database with a datetime column, you dump it (rails db:schema:dump) and then load schema (rails db:schema:load), is the result a datetime column?

I couldn't check that. But it seems that datetime_nil_precision_col and datetime_col are equally dumped. That makes me think that my expectations of dumping & loading are not met.

Sorry if I'm not making myself clear.

aidanharan commented 1 year ago

@mgrunberg The issue was with the assert_line method. If you passed an option with a nil value (like precision: nil) into assert_line then it was considered valid if either:

  1. The column's schema line had that option with a value of nil. { precision: nil } == { precision: nil }. Or,
  2. The column's schema line did not have that option. { precision: nil } == {}.

I have updated assert_line so the options passed in must match all of the schema line options.

mgrunberg commented 1 year ago

@aidanharan amazing! I don't have other concerns. LGTM