From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 11419469710 for ; Mon, 1 Jun 2020 20:47:15 +0300 (MSK) Date: Mon, 1 Jun 2020 20:38:48 +0300 From: Igor Munkin Message-ID: <20200601173848.GU21558@tarantool.org> References: <2fe2a41b8028623b6e5d72c59be716b81dce1e37.1590764167.git.sergeyb@tarantool.org> <20200601133715.px7reef7xmhrdbfx@tkn_work_nb> <20200601163751.GT21558@tarantool.org> <20200601171356.qol3fnd45ehj2qul@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200601171356.qol3fnd45ehj2qul@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, 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. > > Aren't real bugfixes (that actually changes present behaviour) separated > from style changes? Those found by Vlad[1] were separated[2][3] and already merged to stable branches. The one found by me[4] in tarantoolctl tests is still mixed in the corresponding patch[5]. > > Don't get what you meant with 'unintentional occurences': if it is not a > bug and not a code style violation, than it it does not look as > something that should be fixed. But if there is an example, where > luacheck reveals a place that is worth to change for, say, readability, > then okay. I was talking about the following scenario: a suppression is enabled source-wide without checking the root cause and masked bugs or 'unintentional occurences' are left in the source code. This is exactly the case we faced with the bugs I mentioned above. Thereby I propose to investigate errors prior to suppress them. Otherwise, as I said before, enabling luacheck looks like cargo cult. I expect no bugs produced by redefenition but can't say it for sure. > > I bet a beer that this certain warning does not give us any real bug. Agree with you here. > > The main advantage of luacheck is preventing some kinds of typos: say, a > typo in a function name in some failure branch, which is not covered by > tests. The tool is definitely useful without the redefinition warning. Precisely. > > WBR, Alexander Turenko. [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015930.html [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015933.html [3]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015934.html [4]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016066.html [5]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/017245.html -- Best regards, IM