-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
queryx: unset empty values #206
Conversation
0d5919c
to
7f4f8e0
Compare
Makefile
Outdated
@@ -13,7 +13,7 @@ GOTEST_CPU := 1 | |||
endif | |||
|
|||
ifndef GOBIN | |||
GOBIN := $(GOPATH)/bin | |||
GOBIN := $(shell go env GOPATH)/bin |
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 missing, let's define GOPATH like GOBIN.
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.
Done
example_test.go
Outdated
@@ -106,17 +107,30 @@ func basicCreateAndPopulateKeyspace(t *testing.T, session gocqlx.Session) { | |||
|
|||
// Insert song using query builder. | |||
insertSong := qb.Insert("examples.songs"). | |||
Columns("id", "title", "album", "artist", "tags", "data").Query(session) |
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.
Please add new examples if appropriate.
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.
I remained your insert statement, just save it in the separate variable. I suppose it's a good example how one statement can be used in situations with all filled params and partially filled.
If you think it's inappropriate in this example, I can remove it.
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 breaks the convention and history.
Feel free to copy the example and fully adjust it to present your usecase.
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.
Ok, added separate example
transformer.go
Outdated
) | ||
|
||
// bindTransformer is called right before binding a value to a named parameter. | ||
type bindTransformer func(name string, val interface{}) interface{} |
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.
Should be exported.
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.
I think we can leave is as Transformer
, it looks generic enough to use it with parsing rows if needed.
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.
Named it as Transformer
transformer.go
Outdated
// UnsetEmpty unsets all empty parameters. | ||
// It helps to avoid tombstones when using the same insert/update | ||
// statement for filled and partially filled parameters. | ||
var UnsetEmpty = func(name string, val interface{}) interface{} { |
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 should be more explicit that this is a transformer.
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.
Initially I though it would be some redundancy in word transformer but ok
queryx.go
Outdated
@@ -136,7 +144,7 @@ func (q *Queryx) BindStructMap(arg0 interface{}, arg1 map[string]interface{}) *Q | |||
return q | |||
} | |||
|
|||
func bindStructArgs(names []string, arg0 interface{}, arg1 map[string]interface{}, m *reflectx.Mapper) ([]interface{}, error) { | |||
func bindStructArgs(names []string, arg0 interface{}, arg1 map[string]interface{}, m *reflectx.Mapper, tr bindTransformer) ([]interface{}, error) { |
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 already has 2 Query fields passed in let's make it a method.
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.
Done
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.
Some comments inline, overall it looks ok.
I'd suggest you add a var DefaultBindTransformer
similar to DefaultUnsafe.
This way it can be applied to the whole app without setting it each time.
7f4f8e0
to
a42f7e7
Compare
// Transformer transforms the value of the named parameter to another value. | ||
type Transformer func(name string, val interface{}) interface{} | ||
|
||
// DefaultBindTransformer just do nothing. |
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 sounds a bit strange but I couldn't come up with anything better :)
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.
I set it as nil to save compatibility with older versions. Maybe someone uses insert/update statements with nil values to explicitly (or implicitly, who knows) delete columns
All is good let's fix the example. The current example is called |
a42f7e7
to
08e1b4a
Compare
Thanks for the contribution! |
@mmatczuk can we tag a new release, so we can update our dependencies? |
Let's do that. |
Resolve #202