Re: [Tarantool-patches] [PATCH luajit] test: limit code and comment max length by 80

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5552 bytes --]

Hi, Sergey!

On 12.02.2025 10:41, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the clarification.
> See my answers below.
>
> On 12.02.25, Sergey Bronnikov wrote:
>> Hi, Sergey
>>
>> thanks for review! Please see my comments.
>>
>> On 11.02.2025 14:51, Sergey Kaplun via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> It's a nice idea to automate this part of the review!
>>>
>>> Please consider my questions below.
>>>
>>> On 11.02.25, Sergey Bronnikov wrote:
>>>> The patch sets a max length with 80 symbols and fixes tests
>>>> exceeding this length.
>>>> ---
>>>> Branch:https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-set-max-length
>>>> Luacheck configuration file:
>>>> https://luacheck.readthedocs.io/en/stable/config.html
>>>>
>>>>    .luacheckrc                                            |  5 +++++
>>>>    test/tarantool-tests/fix-argv-handling.test.lua        |  4 ++++
>>>>    .../gh-3196-incorrect-string-length.test.lua           |  3 +++
>>>>    test/tarantool-tests/gh-6163-min-max.test.lua          |  4 ++++
>>>>    .../lj-366-strtab-correct-size.test.lua                | 10 ++++++++--
>>>>    .../lj-494-table-chain-infinite-loop.test.lua          |  3 ++-
>>>>    test/tarantool-tests/lj-688-snap-ir-rename.test.lua    |  2 ++
>>>>    test/tarantool-tests/lj-819-fix-missing-uclo.test.lua  |  6 ++++--
>>>>    .../lj-918-fma-numerical-accuracy-jit.test.lua         |  4 ++++
>>>>    .../lj-918-fma-numerical-accuracy.test.lua             |  4 ++++
>>>>    test/tarantool-tests/or-144-gc64-asmref-l.test.lua     |  4 ++++
>>>>    11 files changed, 44 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/.luacheckrc b/.luacheckrc
>>>> index f2573e42..8047677c 100644
>>>> --- a/.luacheckrc
>>>> +++ b/.luacheckrc
>>>> @@ -3,6 +3,11 @@ std = 'luajit'
>>>>    -- This fork also introduces a new global for misc API namespace.
>>>>    read_globals = { 'misc' }
>>>>    
>>>> +max_line_length = 80
>>> I suppose this is excess since we have all other settings intact.
>> Removed.
>>>> +max_code_line_length = 80
>>>> +max_string_line_length = 80
>>> Do we need this particular check? Maybe it is covered by the
>>> `max_code_line_length` and `max_line_length`. Can we avoid markers in
>>> <gh-3196-incorrect-string-length.test.lua> without it?
>>>
>>>> +max_comment_line_length = 80
>>> Actually, the comment length for the Lua part is 66 symbols. Maybe we
>>> may omit this check to avoid stubs in the comments?
>> The main idea is automating this check and avoid comments like
>> "your comment is exceeded XXX symbols" on review. So I don't understand
>> how to omit this check.
> Sorry, I misled you with bad wording.
> Originally, according to the commit message. I think that we would
> change the limit to 80 symbols for all code (and check comments manually
> or don't check them at all). But since we want to do it fully
> automated,
> we should use the following 2 lines:
> | max_code_line_length = 80
> | max_comment_line_length = 66
Well, did it in updated patch.
>
> Or we still should check manually the comment line length on the review.
>
> We don't need `max_line_length`, since we distinguish lines with/without
> comments.
> And we don't need string lines, I suppose. At least I don't fully
> understand why we need them.
both removed from config.
>
>> BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it
>> means
> Yep, totally 118 warnings for the code we want to check.

All warnings were fixed and patch force-pushed to the branch.

See v2 version in ML.

>
>> that manual (visual) checking of max length in comments is unreliable
>> and we definitely
>> should automate it.
> Totally agree.
>
>>>> +
>>>>    -- The `_TARANTOOL` global is often used for skip condition
>>>>    -- checks in tests.
>>>>    files['test/tarantool-tests/'] = {
>>>> diff --git a/test/tarantool-tests/fix-argv-handling.test.lua b/test/tarantool-tests/fix-argv-handling.test.lua
>>>> index 84b626c3..5f147197 100644
>>>> --- a/test/tarantool-tests/fix-argv-handling.test.lua
>>>> +++ b/test/tarantool-tests/fix-argv-handling.test.lua
>>>> @@ -10,7 +10,11 @@ test:plan(1)
>>>>    -- to a single empty string if it is empty [1], so the issue is
>>>>    -- not reproducible on new kernels.
>>>>    --
>>>> +-- luacheck: push no max comment line length
>>> Side note: Can it will be done without push/pop interface? Like for
>>> globals we use:
>>> | luacheck: no global
>> AFAIR, no. Global suppression will cover the whole file, I don't think
>>
>> it is a good idea. Approach with push/pop was already used in
>>
>> commit 9ac79bd7cd12dc3dbfa0e502e29a99b3083676c1
>>    ("test: introduce test:done TAP helper") why we cannot continue using
>> this approach for other files?
> I'm not against it. I just prefer the inline suppressions.
> Actually we _may_ use
> | -- luacheck: no max_comment_line_length
> But this is a global (the whole file below this line) suppression.
> I thought that this suppresses warnings until the next line of code, like
> ignore does. BTW,
> | -- luacheck: ignore
> does exactly what I want, but unfortunately it may not be clear what
> exactly we ignore :).
>
> But this is a matter of taste, so feel free to choose any option here.
>
Discussed it in a private conversation and decided a variant with push/pop

and a warning name with underscores instead whitespaces.

>>>> +--
>>>>    -- [1]:https://lore.kernel.org/all/20220201000947.2453721-1-keescook@chromium.org/
>>>> +--
>>>> +-- luacheck: pop
>>>>    
> <snipped>
>
>>>> -- 
>>>> 2.34.1
>>>>

[-- Attachment #2: Type: text/html, Size: 8763 bytes --]