Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

max_string_line_length not working as expected #168

Open
yangm97 opened this issue Jul 2, 2018 · 5 comments
Open

max_string_line_length not working as expected #168

yangm97 opened this issue Jul 2, 2018 · 5 comments

Comments

@yangm97
Copy link

yangm97 commented Jul 2, 2018

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)

@mpeterv
Copy link
Owner

mpeterv commented Sep 12, 2018

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.

@yangm97
Copy link
Author

yangm97 commented Sep 14, 2018

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
Copy link
Author

yangm97 commented Sep 14, 2018

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.

@mpeterv
Copy link
Owner

mpeterv commented Sep 14, 2018

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.

@chris-jansson
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants