Add lll linter to golangci-lint configuration

[gijswijs]

The lll linter forces the code to be formatted with a maximum line length of 80 and a tab width of 8.

The deadline configuration is replaced with timeout. deadline is deprecated since v1.20 and will be removed in v1.57.0.

All line lengths in the repo are fixed where necessary, or ignored by adding a nolint:lll directive.

[guggero]

Thanks for the effort!
Unfortunately, I think it was a bit premature. Mostly because I think this could also be solved with just a visual guide in the IDE, if the linter not being active is a blocker for your workflow (maybe this message got lost in chat yesterday). Secondly, the updated code should follow our lnd formatting rules (which apply to all our projects), so you'll have to touch a large portion of the diff again...

[guggero]

I'm not sure if spending time on this is really worth doing right now...
My suggestion would be to instead commit the .vscode/settings.json file to the repo so a ruler is automatically shown (see https://github.com/lightningnetwork/lnd/tree/master/.vscode for example).

Also, please take a look at our formatting rules, we want a much more compact formatting of the function headers: https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md

[gijswijs]

I have the visual guide active in the IDE. But that doesn't prevent little slip ups. There were literally hundreds of slip ups in the existing code base already. Also lll is active in lnd and I think it makes sense to have this aligned across repos.

I'm aware of the formatting rules. The function headers you refer to were done automatically with https://github.com/segmentio/golines. Let me see if I can fix those automatically as well.

Or if we choose not to, that's ok with me as well. I thought the consensus was that it was worth the effort.

[guggero]

I have the visual guide active in the IDE. But that doesn't prevent little slip ups. There were literally hundreds of slip ups in the existing code base already. Also lll is active in lnd and I think it makes sense to have this aligned across repos.

Yes, I know we have quite a few instances where we don't really adhere to the implicit rule. Though in some cases wrapping at 80 characters makes the code quite ugly and less readable. I guess in such cases we should use the //nolint directive rather than trying to bend the code too much.
The main reason we have turned on the linter in lnd is that we have a lot of external contributors there, which often don't look at the formatting rules at all.

I'm aware of the formatting rules. The function headers you refer to were done automatically with https://github.com/segmentio/golines. Let me see if I can fix those automatically as well.

Unfortunately I haven't found a tool that can format the function definitions and calls exactly the way we want... If you do find one that can be configured to what we want, that would be fantastic.

Or if we choose not to, that's ok with me as well. I thought the consensus was that it was worth the effort.

After looking through the PR quickly and seeing how many instances there are (and how many of them I would've disagreed with and/or commented on), I think we perhaps should only fix the most ugly ones (where things are too long by a lot of charactesr) and for the rest just configure the linter to start enforcing from a certain commit onward...

[gijswijs]

Slightly offtopic for this PR: But our formatting rules state nothing about linebreaks after dots, e.g.:

unsupportedTapLeafVersion = txscript.
        TapscriptLeafVersion(0xf0)

Allowed or heresy?

[guggero]

Slightly offtopic for this PR: But our formatting rules state nothing about linebreaks after dots, e.g.:

unsupportedTapLeafVersion = txscript.
        TapscriptLeafVersion(0xf0)

Allowed or heresy?

I would say allowed but I personally try to avoid that style at all cost, as it's quite hard to read IMO.
Tricks to avoid can be to shorten the call object (e.g. if you have c.cfg.Wallet.Controller.MyMethod() then you could create a temporary variable ctrl := c.cfg.Wallet.Controller, then just call ctrl.MyMethod(). Or shorten the returned variable name, so just unsupportedVersion := txscript.TapscriptLeafVersion(...) in your example.

[guggero]

Nice, thanks for the update. I prefer this approach. We can still fix existing issues in follow-up PRs.

[guggero]

nit: could be squashed with previous commit.

[jharveyb]

LGTM, TIL new-from-rev 👍🏽