From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E914B469710 for ; Mon, 1 Jun 2020 19:46:17 +0300 (MSK) Date: Mon, 1 Jun 2020 19:37:51 +0300 From: Igor Munkin Message-ID: <20200601163751.GT21558@tarantool.org> References: <2fe2a41b8028623b6e5d72c59be716b81dce1e37.1590764167.git.sergeyb@tarantool.org> <20200601133715.px7reef7xmhrdbfx@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200601133715.px7reef7xmhrdbfx@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v6 08/25] Fix luacheck warnings in test/app-tap List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Sasha, Thanks for joining to review! On 01.06.20, Alexander Turenko wrote: > > -local function execute_and_verify(test, client, input, exp_output, name) > > - test:test(name, function(test) > > +local function execute_and_verify(testcase, client, input, exp_output, name) > > + testcase:test(name, function(test) > > Please, don't forbid redefinitions. I often use it for 'standard' names > like 'opts', 'ok', 'err', 'test'. It allows to write code blocks in the > same style: > > | local ok, err = <...> > | <...> > | local ok, err = <...> > I have no objections regarding this proposal, only two nits: * Those suppressions should be enabled source-wide *only and only when* all unintentional occurences or the ones masking bugs (if we found such) are fixed. Otherwise I see no sence in enabling luacheck. * You can use blocks to write code *blocks* in the same style. I hope you won't take this comment as an intrusive guidance, it's just another approach I found for your problem. > Instead of lopsided way (which also give additional problems, when you > change such code): > > | local ok, err = <...> > | <...> > | ok, err = <..> > > Or with artificial declarations at block start: > > | local ok, err > | <...> > | ok, err = <...> > | <...> > | ok, err = <...> > > I don't like the requirement to use 'testcase', 'testcasecase' or > 'test2', 'test3' to name usual 'test' variable. This change relates to a shadowing warning. Vlad is strictly against to fixing such occurences. I can live both with or without it. Since you're also against, I guess Sergey need to revert such changes for the cases *it is made intentionally* and then suppress this warning source-wide. > > WBR, Alexander Turenko. -- Best regards, IM