Rule request: Comment lines in function bodies

[driver733]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

Rule Request

Description

The code, in my opinion, should not have in-line comments. If any kind of explanations is needed, it has to be put in the method or class documentation. Furthermore, if your code requires comments, then it might be necessary to refactor it, so that it would be self-explainatory.

What would trigger?

internal func exceedsLineCountExcludingCommentsAndWhitespace(_ start: Int, _ end: Int, _ limit: Int) -> (Bool, Int) {
        guard end - start > limit else {
            return (false, end - start)
        }
        // I will put this here instead of putting in the method doc, where it belongs.
        let count = end - start - numberOfCommentAndWhitespaceOnlyLines(startLine: start, endLine: end)
        return (count > limit, count)
    }

What would not trigger?

internal func exceedsLineCountExcludingCommentsAndWhitespace(_ start: Int, _ end: Int, _ limit: Int) -> (Bool, Int) {
        guard end - start > limit else {
            return (false, end - start)
        }
        let count = end - start - numberOfCommentAndWhitespaceOnlyLines(startLine: start, endLine: end)
        return (count > limit, count)
    }

Configurable?

The rule should not be configurable.

Opt-in or enabled?

This rule could be a personal preference or coding style from various individuals, so it could be kept as opt-in.

[fredpi]

I'm strictly against this (even when done as an opt-in rule). While I agree that inline method comments often can be a doc-comment on top of the method declaration, I don't believe disallowing comments entirely within method is somehow encouragable.

While comments are often used to document code that is poorly written and that would be self-explanatory if implemented properly (one common example being the (mis)use of a UIView's tag attribute), there are other use cases for comments:

• Grouping blocks. Probably the most important use of comments in my opinion. Even within a single method, there are cases where you wouldn't want to split things up into other singular methods, but instead group lines together and write a comment above the group which explains the context (consider it some kind of headline).
• Documenting hacks. The notion that good code never needs explaining comments is not entirely true in my opinion. If one doesn't have complete control over systems and APIs in use, it's sometimes needed to apply hacky approaches to issues that need to be overcome. As for me, I experienced a severe issue with a WKWebView (acknowledged to be a bug by Apple) which could be solved by adding some large empty div in the html and then subtracting the height of the div from the contentInset in code. This approach was very much needed and so was the comment explaining it. There are quite many such cases and I don't believe adding // swiftlint:disable ... everywhere would be appropriate here.

Given those two scenarios, I don't think such a rule should be added, not even opt-in which nonetheless implied it's something to be encouraged.

[driver733]

@fredpi

1. Grouping blocks

The methods (and classes) are supposed to be small and fulfill only one goal. If your method has "parts", then it probably does several things and should be split. If you cannot split a single method into separate ones, then you have to rework your design.

2. Documenting hacks

If your API forces you to use a hack, you need to properly document in the class documentation header. You need to inform the users what the bug is about, how you fixed it and why. You should also leave a link to the bug report website. All of this belongs to the documentation section, not the code itself.

[fredpi]

1. No, it's not always (sometimes of course) good to make functions do a single thing in my opinion. A function is a logical unit, so is a group with a comment as its heading. In Swift there are several layers of abstraction (Types, Subtypes, Functions, Inline Functions). If possible, this pattern should be used to employ a hierarchic structure. Splitting a method into sub methods always creates a cut in this structure which can easily be overcome by using comments to summarise small logical groups.

2. Documentation headers are shown when the user inspects the function. They should be as small as possible yet deliver every information needed to use the function properly. A hack does not fit in this category most of the time. You don't really want information about a part of the inner workings of this method in a documentation header publicly available. It would be confusing and simply not in its proper place.

[SDGGiesbrecht]

@fredpi,

Documentation headers are shown when the user inspects the function. [...] It should be confusing and simply not put properly.

Das hat mich gerade zum Lachen gebracht. 😄 Es bedeutet sowas wie „Es soll verwirrend und einfach nicht richtig formuliert sein.“ – ganz das Gegenteil von dem, was Sie meinen: “It would be confusing and simply not in its proper place.”/„Es wäre verwirrend und einfach nicht an der richtigen Stelle.“

Aber ich habe erst am letzten Satz gecheckt, dass es von einem Fremdsprachiger geschrieben wurde, also haben Sie es recht gut hingekriegt. (Vergeben Sie mir in dem Fall, dass Sie nur vertippt haben, Englischsprachiger sind, und dazu nur zufällig in Karlsruhe wohnen. Man weiß ja nie ...)

Ach und übrigens bin ich ganz Ihrer Meinung.

[fredpi]

@SDGGiesbrecht Danke für den Hinweis, ist korrigiert ;)

[Jeehut]

@fredpi While I understand that you don't consider this idea a "best practice" (I also don't), I can still imagine a programming style where a function is the smallest possible unit and should always have a clear goal – and if anything as part of that function needs explanation or needs to be grouped, that should be put into a separate function. Of course, this programming style has it's own drawbacks, but before we get into further discussions here I'd like it to be clear what SwiftLint is about so we can maybe just close this or merge it although we don't think it's a best practice widely used in the community:

@marcelofabri @jpsim Like I already asked here the big question is if SwiftLint should also include opt-in rules that enable enforcing programming styles that are not considered best practice by a wide part of the Swift community but still is a coding style a minority might decide to use. What are your opinions on this? Because I feel like this is the case here and we should be very clear about it so we can either close this or merge it although we might not agree it to be a best practice.

[Jeehut]

Looking at SwiftLint's own description, which is "A tool to enforce Swift style and conventions" to me it sounds like we should include any rule that might be a reasonable Swift syle or convention as long as there's no "definitely better way" – and I don't think @driver733's idea suggested here can be clearly stated as a "bad style" since his rationale does make sense. Some exceptions of course should be made, for example for rarely used conventions that might make this project too much harder to maintain. But that's a different discussion.

[marcelofabri]

I think it's fine to have it as opt-in as realistically, plugins are not coming so soon.

[fredpi]

@Dschee That rationale seems reasonable, yet I still believe that the decision to include rules that aren't considered best practice by most, should only applied to rules that cannot be defined via a custom regex rule... If you can define a rule via a regex (which I believe is the case here) and it's only needed by few projects, I don't see a need to include it as a part of SwiftLint...