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

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

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

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.

> 
> BTW luacheck with "max_comment_line_length = 66" reports 89 warnings, it 
> means

Yep, totally 118 warnings for the code we want to check.

> 
> 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.

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

<snipped>

> >> -- 
> >> 2.34.1
> >>

-- 
Best regards,
Sergey Kaplun