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 D57C8469710 for ; Tue, 2 Jun 2020 19:07:14 +0300 (MSK) Date: Tue, 2 Jun 2020 18:58:06 +0300 From: Igor Munkin Message-ID: <20200602155806.GE5745@tarantool.org> References: <20200602144255.mwseuua4qpsy3unw@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200602144255.mwseuua4qpsy3unw@tkn_work_nb> Subject: Re: [Tarantool-patches] [PATCH v6 00/25] Add static analysis with luacheck List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, o.piskunov@tarantool.org Sasha, Thanks for your summary! I'd love to give my two cents. On 02.06.20, Alexander Turenko wrote: > I was asked to summarize my proposals re this patchset. > > 1. Disable redefinition and shadowing warnings globally (in a config) > and don't touch 'affected' code. I provided examples why those > changes are not good. > 2. Don't remove unused arguments, where they show a function contract: > use '-- luacheck: no unused args' or even disable them globally if > there are many of them. > > Personally I use 'luacheck -r' with default config and just visually > filter out warnings re 'box' global and so. Before I use '--no-redefined > --no-unused-args' in a commercial project (24K sloc in Lua now) and in > tarantool/graphql.0 (8K sloc, +15K sloc of tests). > > I don't think that reverting or applying of a hundred of hunks is so big > work to discuss whether it worth to don't do this, because amount of > work is more then profit, despite that unneded diffs are discouraged, > some code will became less readable, git blame will show unnecessary > artificial changes, but we'll save a day or even two, but our code > examples would suggest to avoid redefinitions and will push future > developers to produce more such code, but two days or ever three, but > maintenance costs are always bigger then developing costs, but... > blah-blah. C'mon. > > Three years ago, when I come into the team, it would not be ever > discussed. If unneded work was made it is would be a signal to think why > it appears and, say, extract doubtful changes into separate commits. Or > keep notes to don't miss parts of related discussions. Or ask possible > reviewers before do large amount of changes. Not 'ask to push it anyway, > because much time was spent'. > > It is still my personal opinion: I don't a reviewer here. #include OK, I am the reviewer, even though I joined review procees only on v3 series. I have already given LGTMs for several first patches, since they are done according to approach I described here[1]. Nobody was against it and the target (i.e. definition of done) implied to prepare our CI pipeline for luacheck, check our sources for errors, fix the trivial ones, suppress false-positive (made by intention) warnings and file follow-ups if necessary. Unfortunately, the definition of done was mentioned neither in the ticket nor in ml. Today, when the v6 series is partly approved we decided to finally discuss what need to be done. As a result the target has been changed a bit and I'm sure we need to adjust the patchset considering new comments and requirements (like we have done in v4). Now, I would like to revoke my LGTMs, but I totally don't want anyone takes this as a sabotage of the series. Since this moment my approvals for this version are just formal and should be considered as "I failed to found the changes leading to platform degradation". Does the patchset *really* LGTM now? No. > > WBR, Alexander Turenko. [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016009.html -- Best regards, IM