From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 1CCE4469710 for ; Tue, 2 Jun 2020 17:43:13 +0300 (MSK) Date: Tue, 2 Jun 2020 17:42:55 +0300 From: Alexander Turenko Message-ID: <20200602144255.mwseuua4qpsy3unw@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: sergeyb@tarantool.org Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, o.piskunov@tarantool.org 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. WBR, Alexander Turenko.