(chore): Add max lll = 120 linter check and fix issues

[camilamacedo86]

No description provided.

[vkhoroz]

This means we're doing something wrong.

The thing is: Fioctl is a CLI tool, meaning it consists by maybe 10% of human-readable text.
That text must never be impacted by e.g. auto-format or linter-fix.

IIRC @kprosise set the expectation for human-readable sentences to never exceed 150 characters.
Let's add 10 characters for syntax overhead (I assume one tab counts as one character).
What it means: try to set the line length limit to e.g. 160 and see what happens.

As is this PR creates too much code friction.
But I do not want to say no to it: it is a good thing, we just need to adapt it to our reality.

[camilamacedo86]

Hi @vkhoroz,

Thank you for your feedback. Let me address the points you've raised:

That text must never be impacted by e.g. auto-format or linter-fix.

The linter fix does not fix this one.

This means we're doing something wrong.

Are you suggesting we limit the size of the long descriptions lines to ensure that output resolution doesn't break the lines? If not, then perhaps we can just overlook the check in such cases.

Also, if we want to ensure the long values of the output then I suggest we just do a follow-up PR where we remove the ignore nolint:lll and fix that. So, there will be fewer changes in one PR and make easier the review.

As is this PR creates too much code friction.

The primary change in this PR was to break the lines to improve code readability and to comply with the linter check (lll). It doesn't modify the actual code logic, which should make it easy to review and compare. If you have any specific concerns, could you please elaborate so I can address them?

But if you want to wait for #278 that is all fine and then we move forward with this one All fine too. 👍

[vkhoroz]

The primary change in this PR was to break the lines to improve code readability and to comply with the linter check (lll).

We never had this requirement.
The code readability is a very opinionated topic; hence we defer this to whatever make format thinks about it.

The linter check needs to comply with our coding style (if possible), not the opposite.
We use longer lines in our code than this new tool tries to enforce.
Still, it is easy to configure the tool to accept longer lines, so we must go for it.

A necessity to tag our help messages with linter skips is cumbersome, hence, not acceptable.
Needless to say, it also decreases the value we derive from that linter.
A fact that this thing doesn't modify the actual code logic while creating a lot of code friction also brings doubtful value.

This has nothing to do with #278.

Simply set the line limit to, say 160; and if it doesn't create another huge diff - we are fine.
Otherwise, I'd set it to whatever other arbitrarily higher value which doesn't result into needless code changes.

[camilamacedo86]

Hi @vkhoroz,

Thank you for your input following the comments inline:

Regards Line Length Agreement == 120:

I incorporated the 120 character limit based on your comment however, that is the default value anyway, I just added it explicitly in the linter for clarity:

What is the whole purpose of the linter?

Code Readability for Maintainability:

Lines exceeding 120 characters typically necessitate horizontal scrolling, thereby impacting code readability and maintenance. Simply adjusting the limit to 160 to fit the current code appears counterintuitive, as it would deviate from the linter's primary objective.

Regards your your note: A necessity to tag our help messages with linter skips is cumbersome, hence, not acceptable

While linters provide skip options for exceptions and I do not understand why use then would be not acceptable, my primary query was: Should we enforce a fixed output size for help messages, or permit dynamic resizing based on terminal width?

Example config:

issues:
  # Excluding existing issues for the lll linter, assuming they match a certain pattern.
  # Here's an example of excluding all long line errors in certain files or with certain patterns
  exclude-rules:
    - path: _test.go
      linters:
        - lll
      source: ".*"
    # Add more patterns or files as necessary
    
  # The `new` configuration applies to all linters, not just `lll`
  new: true

Regards your your note: too much code friction

If the chief concern centers around widespread alterations across files, we have the flexibility to set the linter to dismiss current discrepancies and solely assess fresh alterations.

Yet, I'm at a loss regarding this apprehension. What issues arise from refactoring numerous files or lines to enhance code clarity? How it could this potentially disrupt the tool? Is there worry over possible conflicts with PRs, such as #278? If that's the case, we can prioritize its merge.

Thank you for the feedback. I value the discussion as it helps refine the PR to best suit our project's needs.

[vkhoroz]

@doanac can you comment on this?

My call is to either (1) table this or (2) set the max length to160 or higher to accommodate our naturally long help messages.

I see no rationale to have a linter which requires disabling it here and there all the time.

[camilamacedo86]

"I'm against (2) set the max length to160 or higher to accommodate our naturally long help messages. We use a linter for short lines, so setting a high max length defeats its purpose.

I support:

(3) - Break and fix the line in the 3 spots.
(4) - Ignore existing issues but block new ones.
(5) - Use the nolint: option to ignore the check when it makes sense since it is the purpose of this feature
(1) - Consider not adding the linter at all

[doanac]

I'd rather not have to think about line lengths right now.