Hi, Sergey!

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.

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