Re: [Tarantool-patches] [PATCH v6 08/25] Fix luacheck warnings in test/app-tap

[Igor Munkin <imun@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 <do ... end> 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