Skip to content

Fix SqlTyper#355

Merged
nelsam merged 1 commit intogo-gorp:masterfrom
icholy:master
Oct 21, 2017
Merged

Fix SqlTyper#355
nelsam merged 1 commit intogo-gorp:masterfrom
icholy:master

Conversation

@icholy
Copy link
Copy Markdown
Contributor

@icholy icholy commented Oct 18, 2017

The SqlType type should be returning a driver.Value instead of driver.Valuer

gorp/db.go

Line 326 in 659d466

gotype = reflect.TypeOf(typer.SqlType())

The way to get it working now is to do something like this

type IntValuer int

func (IntValue) Value() (driver.Value, error) {
  return nil, nil
}

type CustomType int

func (CustomType) SqlType() driver.Valuer {
  return IntValuer(0)
}

This will continue working with the proposed change.

@nelsam
Copy link
Copy Markdown
Member

nelsam commented Oct 21, 2017

One last request: can we do log.Printf("Deprecation Warning: update your SqlType methods to return a driver.Value") in the logic for legacySqlTyper?

@icholy
Copy link
Copy Markdown
Contributor Author

icholy commented Oct 21, 2017

@nelsam done and squashed.

@nelsam
Copy link
Copy Markdown
Member

nelsam commented Oct 21, 2017 via email

@icholy
Copy link
Copy Markdown
Contributor Author

icholy commented Oct 21, 2017

Messed up the rebase, fixed now.

@icholy
Copy link
Copy Markdown
Contributor Author

icholy commented Oct 21, 2017

In regards to changing the null types, I think that will break compatibility with sql.Null<Name> types.

@nelsam nelsam merged commit dc4f2a4 into go-gorp:master Oct 21, 2017
@nelsam
Copy link
Copy Markdown
Member

nelsam commented Oct 21, 2017

Yeah. We can maybe update our code to be more aware of how those types usually work, make our code more generic instead of checking the type name for everything, but we have no guarantee that it'll work for all use cases.

Back when we were more active, we had plans to separate the database setup logic and make it a bit more explicit, but it's become difficult to put that much time in to gorp for most of us.

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.

2 participants