Re: [Tarantool-patches] [PATCH v6 00/25] Add static analysis with luacheck

[Igor Munkin <imun@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 <disclaimer.h>

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