-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix opts table damaging by CRUD methods #200
Conversation
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.
Thanks for the patch!
|
||
-- insert_object | ||
local insert_opts = {timeout = 1, bucket_id = 477, fields = {'name', 'age'}} | ||
local new_insert_opts, err = g.cluster.main_server.net_box:eval([[ |
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.
Why do you use eval()
instead of call()
?
-- insert_object
local result, err = g.cluster.main_server.net_box:call(
'crud.insert_object', {
'customers',
{id = 1, name = 'Fedor', age = 59},
insert_opts
}
)
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 I would use call()
I could not catch modification of opts table
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.
Thanks for your patch. See my two suggestions.
0415979
to
2d65dc6
Compare
2d65dc6
to
6af633c
Compare
crud/common/utils.lua
Outdated
@@ -524,4 +524,17 @@ function utils.flatten_obj_reload(space_name, obj) | |||
return schema.wrap_func_reload(flatten_obj, space_name, obj) | |||
end | |||
|
|||
function utils.make_options(opts, add_space_schema_hash_value) |
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 seems to me that the word create
is more suitable here (not make
), or get
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.
make_option
name is often used for parsing options (in Python modules for example). Thats why it seems to me that this name is suitable.
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.
A bit more bikeshedding: it looks specific for the add_space_schema_hash
option, so it would be better either choose a specific name or rewrite the function to be common:
-- Merge two options map.
--
-- `a` and/or `b` can be `nil`.
--
-- If `a.foo` and `b.foo` exists, prefer `b.foo`.
function utils.merge_options(a, b)
return fun.chain(a or {}, b or {}):tomap()
end
So the call will be the following:
-opts = utils.make_options(opts, true)
+opts = utils.merge_options(opts, {add_space_schema_hash = true})
Maybe a bit more Lua tables, but the call becomes more readable (now we see what is the true/false value).
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.
Updated:
+-- Merge two options map.
+--
+-- `opts_a` and/or `opts_b` can be `nil`.
+--
+-- If `opts_a.foo` and `opts_b.foo` exists, prefer `opts_b.foo`.
+function utils.merge_options(opts_a, opts_b)
+ return fun.chain(opts_a or {}, opts_b or {}):tomap()
+end
6af633c
to
66cc052
Compare
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.
LGTM (but there is a nitpicking around naming, see below).
If we have
opts
variable and we pass it to some crud method(
upsert_object
for example) and then useopts
variable againfor crud method (
get
for example) we receive an error because*_object
method damagesopts
variable addingadd_space_schema_hash
parameter toopts
variable.Closes #192