Re: [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error created via box.error.new to diag

[Nikita Pettik <korablev@tarantool.org>]

On 27 Mar 01:19, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 2 comments below.
> 
> On 25/03/2020 02:43, Nikita Pettik wrote:
> > To achieve this let's refactor luaT_error_create() to return error
> > object instead of setting it via box_error_set().
> > luaT_error_create() is used both to handle box.error() and
> > box.error.new() invocations, and box.error() is still expected to set
> > error to diagnostic area. So, luaT_error_call() which implements
> > box.error() processing at the end calls diag_set_error().
> > It is worth mentioning that net.box module relied on the fact that
> > box.error.new() set error to diagnostic area: otherwise request errors
> > don't get to diagnostic area on client side.
> > 
> > Needed for #1148
> > Closes #4778
> > 
> > @TarantoolBot document
> > Title: Don't promote error created via box.error.new to diagnostic area
> > 
> > Now box.error.new() only creates error object, but doesn't set it to
> > Tarantool's diagnostic area:
> > ```
> > box.error.clear()
> > e = box.error.new({code = 111, reason = "cause"})
> > assert(box.error.last() == nil)
> > ---
> > - true
> > ...
> > ```
> > To set error in diagnostic area explicitly box.error.set() has been
> > introduced. It accepts error object which is set as last system error
> > (i.e. becomes available via box.error.last()).
> > Finally, box.error.new() does not longer accept error object as an
> > argument (this was undocumented feature).
> > Note that patch does not affect box.error(), which still pushed error to
> 
> 1. 'pushed' -> 'pushes'.
> 
> > diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> > index 640e33910..ff285d7eb 100644
> > --- a/src/box/lua/error.cc
> > +++ b/src/box/lua/error.cc
> > @@ -139,9 +144,12 @@ luaT_error_new(lua_State *L)
> >  {
> >  	if (lua_gettop(L) == 0)
> >  		return luaL_error(L, "Usage: box.error.new(code, args)");
> > -	luaT_error_create(L, 1);
> > +	struct error *e = luaT_error_create(L, 1);
> > +	if (e == NULL)
> > +		return luaL_error(L, "box.error.new(): bad arguments");
> 
> 2. I think it would be better to return the same 'Usage' error
> in case of a problem with arguments. Not two different messages
> meaning essentially the same. Or both should return 'bad arguments'.
> However with both 'Usage' the diff would be 1 line smaller.
> 
> Other than these comments, LGTM. If you fix them both - then push right
> away.

Pushed to master, updated changelog.