Skip to content
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

make the NullTime type an alias of sql.NullTime #1049

Closed

Conversation

shogo82148
Copy link
Contributor

Description

updates #960

currently, the following code will fails because of type miss match.

func doSomething() mysql.NullTime

var nt sql.NullTime
nt = doSomething() // compile error

It may prevent migrating from mysql.NullTime to sql.NullTime.
From Go 1.9, Go supports type aliases. https://golang.org/doc/go1.9#language
It helps migrating.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@julienschmidt
Copy link
Member

Something seems to be broken with Go13+ (see Travis):

--- FAIL: TestRowsColumnTypes (0.04s)

    driver_test.go:2910: failed to scan values in sql: Scan error on column index 38, name "datetime": unsupported Scan, storing driver.Value type []uint8 into type *time.Time
--- FAIL: TestScanNullTime (0.00s)

    nulltime_test.go:53: 2012-06-14: expected error status false, got true

    nulltime_test.go:53: [50 48 49 50 45 48 54 45 49 52]: expected error status false, got true

    nulltime_test.go:53: 2011-11-20 21:27:37: expected error status false, got true

    nulltime_test.go:53: [50 48 49 49 45 49 49 45 50 48 32 50 49 58 50 55 58 51 55]: expected error status false, got true

    nulltime_test.go:53: 0000-00-00: expected error status false, got true

    nulltime_test.go:53: [48 48 48 48 45 48 48 45 48 48]: expected error status false, got true

    nulltime_test.go:53: 0000-00-00 00:00:00: expected error status false, got true

    nulltime_test.go:53: [48 48 48 48 45 48 48 45 48 48 32 48 48 58 48 48 58 48 48]: expected error status false, got true

    nulltime_test.go:56: : expected valid status false, got true

    nulltime_test.go:56: 1234: expected valid status false, got true

    nulltime_test.go:56: 0: expected valid status false, got true

@julienschmidt julienschmidt added this to the v1.5.0 milestone Jan 5, 2020
case time.Time:
nt.Time, nt.Valid = v, true
return
case []byte:
Copy link
Member

Choose a reason for hiding this comment

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

parseDateTime for []byte and string are still required, even with sql.NullTime in Go 1.3+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is misuse of mysqlNullTime, users should add the parseTime=true option.
because parseDateTime does not consider timezones.
TestLegacyNullTime and TestNullTime are working examples.

but, you know, it may break existing codes. I'm thinking about it now... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

We should at least not push it right before a release. Let's reconsider this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree

@julienschmidt julienschmidt modified the milestones: v1.5.0, v1.6.0 Jan 6, 2020
@julienschmidt julienschmidt modified the milestones: v1.6.0, sequel May 22, 2020
@dolmen
Copy link
Contributor

dolmen commented May 28, 2020

You probably don't need that much this change for your migration path. Here is a simple workaround: a type cast, because the representations are now the same.

nt = sql.NullTime(doSomething())

See this code on the Go playground.

@shogo82148
Copy link
Contributor Author

close this because this pull request breaks the compatibility, and it is enough to add a deprecated message into mysql.NullTime.

@shogo82148 shogo82148 closed this May 31, 2020
@shogo82148 shogo82148 deleted the make-nulltime-type-alias branch May 31, 2020 03:54
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.

None yet

3 participants