luacheck icon indicating copy to clipboard operation
luacheck copied to clipboard

max_string_line_length not working as expected

Open yangm97 opened this issue 7 years ago • 5 comments

So I have max_string_line_length = false in my .luacheckrc but:

screen shot 2018-07-02 at 13 59 11

Gets frowned upon. Also, starting a multiline string work as expected, but closing doesn’t:

screen shot 2018-07-02 at 13 59 41

(moving the strings or the parenthesis doesn’t change anything)

yangm97 avatar Jul 02 '18 17:07 yangm97

max_string_line_length applies to lines with their line ending within a string, so this works according to documentation but it is indeed probably not the most useful behavior. I'm not sure how to define a better behavior exactly. Which of these lines should be considered "string" lines?

local value1 = "11111111111111111111111111111111111111111111111111111111111"
local value2 = f("11111111111111111111111111111111111111111111111111111111")
local value3 = function_with_a_very_long_name_that_goes_on_and_on_and_on("")
local value4 = {key1 = "foo", key2 = "Some string 1234567890123", key3 = ""}
local value5 = {{{{{"1111111111111111111111111111111111111111111111111"}}}}}

Considering lines ending with a string ending plus optionally some sequence of closing parens and brackets and commas as "string" lines makes sense, but that also includes cases like line 3 and 4 above where the string at the end doesn't really contribute to the line length.

mpeterv avatar Sep 12 '18 14:09 mpeterv

What if it just skipped string chars altogether? Not sure about the performance implications of that, but sounds like it would be the most consistent way.

So 3 would trigger the warning because of the long function name but the rest wouldn't.

yangm97 avatar Sep 14 '18 06:09 yangm97

I know it's a bit offtopic but, while we're at it, another "unexpected" behavior happens when ignoring comments length. I thought it would ignore only the comment char count but it ends up ignoring the whole line lenght... maybe it's a little late to change the behavior for that but maybe an option for it would be interesting.

yangm97 avatar Sep 14 '18 06:09 yangm97

Ignoring characters of some type (string or comment) when counting line length is an interesting idea. It works when completely ignoring strings or comments, particularly for some specific block of code using inline options.

There is still the use case of enforcing some line length limit (less strict than the general one) for string and comment lines, both for a specific block of code or across the entire codebase. I'm not sure how that idea could be applied here. Ignoring characters of a type up to some given limit is possible but is a roundabout way to configure this, having an option explicitly setting the line limit seems better.

So to me it still seems that improving the rules for determining whether a line is a comment or a string line is the way forward. Perhaps in addition to lines ending within a string or a comment (the current rule) it should also consider lines with strings/comments taking up more than half of the line, or some other heuristic.

mpeterv avatar Sep 14 '18 08:09 mpeterv

Encountering similar confusion with this setting--what if it worked just for lines that had nothing but a string? Such as:

local s = 
  "111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111"

That line is literally nothing but a string, but still gets marked as too long. Seems to me that should be accepted

chris-jansson avatar Nov 13 '18 20:11 chris-jansson