Feature: Ignore comments in getLines()

[martialblog]

This change removes comments from being counted in getLines(). I also added some unittests to make sure it is acutally working.

This was acutally my first deep dive into the go/ast and go/token package, so if there's a better way, please let me know.

Fixes #12

[Dego1n]

@NiseVoid can this be merged?

[NiseVoid]

I no longer work at this company, so I can't merge PRs. Tho I think the change mostly looks fine, it might be good to add some more complex testcases, like functions with code and comments on the same like. And maybe a setting for it would be useful (that would then require a change in golangci-lint), tho at the same time I don't think giving people reasons to document less is a solid idea.

[martialblog]

Hi, an on/off setting seems a good idea. I'll take care of it

So who do we bother to get this merged?

Cheers

[martialblog]

Given it some thought, I think it is fine if comments are just ignored. I would think that people would not count comments in any form to the length of a function.

This also allow for a more seamless integration with golangci-lint

[ldez]

Given it some thought, I think it is fine if comments are just ignored. I would think that people would not count comments in any form to the length of a function.

The problem is related to the meaning of this linter: it counts the lines (code, empty lines, comments).

I am not able to explain why this can be a better approach than cyclomatic complexity. I don't know what was the idea behind the fact to count lines.

But as it counts lines, every line (code, empty lines, comments) should be counted.

We need to know the idea behind this.

Maybe @NiseVoid can explain it.

[robinknaapen]

@ldez

I see that this is a wanted feature. Noted that this is a fundamental change that will probably trigger nolintlint.

Is this acceptable from golangci's perspective? I read the https://golangci-lint.run/usage/install/#versioning-policy. If we accept this PR then golangci will release this in the patch or minor release?

[ldez]

I see that this is a wanted feature.

I think you don't really get my point: if you start to count LOC, funlen will have the same behavior as other linters, it will be a kind of duplicate. If it's a duplicate we have a problem with the meaning of this linter.

why exclude comments instead of empty lines? IMHO empty lines are more a problem than comments, if I understand the idea behind funlen.

We need to know what was the idea behind it to be able to take the right decision.

I think it's a common question https://github.com/ultraware/funlen/issues/6

If we accept this PR then golangci will release this in the patch or minor release?

The releases of golangci-lint are not impacted by linter changes (i.e. we don't create releases only for one linter), you just have to follow semver. But compatibility options are always a good idea :wink: .

[robinknaapen]

@ldez

Sorry, your comment came while I was typing up my comment.

We need to know what was the idea behind it to be able to take the right decision.

I will ask my colleagues and @NiseVoid if they can recall the why

[martialblog]

My two cents on this: I came across this because I wanted something that would notify about a function that might grow too long (assuming that long equals complex).

As the golangci-lint docs state, funlen is a "Tool for detection of long functions" (my assumption was that meant actual executed lines not comments). Another path forward would be, to leave the behaviour of funlen as is, but extend the help text and add an option to disable comments.

I do agree that other measures like gocognit or gocyclo are more precise and probably preferable.

[NiseVoid]

The original intent for the lines check was to fit a function within 1 screen or have only a limited amount of scrolling. If you need to scroll trough a long function, tracing variables back to their definition or even just finding matching brackets becomes extremely difficult. Besides checking lines there's also a separate check for the number of statements, which gives a clearer idea of how much is actually being done in the function. Iirc the values we used internally were something like 50-60 lines and 30-40 statements.

Comments present an interesting challenge here, because depending on how comments are used inside functions they might partially mitigate this problem (for example if you put a few lines of comments per block of code). But comments can also easily add to the chaos of long functions, especially with the "write comments so copilot can generate code" thing some developers do now. Adding setting for it would definitely be the safe bet here, with the default being the old behavior. Adding a setting would however require adding that setting on the golangci-lint side

I am not able to explain why this can be a better approach than cyclomatic complexity.

Rather than being better, it's more about adding more metrics so each can get a more reasonable value. Cylcomatic complexity really hates functions with many if condition checks but hardly cares if you make a long list of functions calls. In both cases you'd want the linter to tell you that a function is getting too big and might need to be split. The linter for cyclomatic complexity was also enabled in the configs, but with a limit that is fairly high and mostly just wards against making one function with way too many branches, and copy-pasting checks for different fields

[ldez]

@robinknaapen I think the answer of NiseVoid should be rephrased and integrated into the readme of the project.

[martialblog]

I opened a PR to add a small summary of the intent in the README.

Regarding the comments, I think a setting to enable/disable the inclusion of comments in the count would be an optional solution. Question I'm asking myself right now is, what the default should be :thinking:

[ldez]

Question I'm asking myself right now is, what the default should be :thinking:

Now we have the meaning: the number of lines is related to screen size, so the default should stay the same.

I recommend adding an option for empty lines too.

[martialblog]

@robinknaapen I updated the code a bit. It now includes an option to toggle the inclusion of comments. Also rebased the branch.

What do you think?

[robinknaapen]

@martialblog

Code lgtm, I just have a followup question below

@ldez Who needs to update the linter call in golangci-lint: https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/funlen.go#L55

Should we open an PR or is this done internally by golangci-lint?

Or should we firstly update the current standard of golangci-lint?

[ldez]

Should we open a PR or is this done internally by golangci-lint?

You just have to tag (v0.1.0), our bot will handle that (every Sunday by default) and I will update the PR created by the bot.