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

Errors for unnecessary space(s) before comments #269

Closed
bethcutler opened this issue Aug 8, 2018 · 13 comments
Closed

Errors for unnecessary space(s) before comments #269

bethcutler opened this issue Aug 8, 2018 · 13 comments

Comments

@bethcutler
Copy link
Contributor

Extra spaces before comments are flagged as errors.

I can't find anything in the Kotlin Style Guide that bans extra spaces before comments. The Android Kotlin style guide specifically allows extra spaces before a comment (see bullet 7).

ktlint allows extra spaces when adjacent lines have aligned comments, but not when those lines are not immediately adjacent (see screenshot). I think multiple spaces should be allowed before a comment even if the comment stands alone, because a double-space before a comment is not uncommon.

ktlint_spaces_errors

@GladeDiviney
Copy link

Under https://kotlinlang.org/docs/reference/coding-conventions.html#horizontal-whitespace,

As a general rule, avoid horizontal alignment of any kind. Renaming an identifier to a name with a different length should not affect the formatting of either the declaration or any of the usages.

So I would think that all of the above should be considered erroneous.

@sschuberth
Copy link
Contributor

On a related note, ktlint 0.33.0 also reports "Unnecessary space(s)" for this line:

https://github.com/heremaps/oss-review-toolkit/blob/73d2cdfc536aa745052ea4a594fe23ce89863457/analyzer/src/main/kotlin/managers/GoDep.kt#L60

So I do get the report also for adjacent lines. Can anyone shed a light on whether this is intended or not?

@shashachu
Copy link
Contributor

Looks like @shyiko removed the adjacent lines check a few months ago in this commit: 696b33e presumably because of the style guide link above. So this does seem to be intended behavior.

@sschuberth
Copy link
Contributor

Yeah, unfortunately that commit message lacks a rationale, so it's really hard to tell.

@ddulaney
Copy link

ddulaney commented Jun 4, 2020

It would be great if there was a separate rule for horizontal alignment of comments. I'd like to disable that one but keep the "Unnecessary space(s)" for code spacing.

@Chesteer89
Copy link

Chesteer89 commented Jul 31, 2020

Any ideas when it'll be fixed? It's really terrible that it has an effect on docs (formatted with tabs):

/**
 * @param message       The payload.
 * @param someParam     Something
 */

Result:
Generating Unnecessary space(s).
Experimental doesn't change anything

@FWDekker
Copy link
Contributor

FWDekker commented Dec 21, 2020

I understand that the style guide recommends against horizontal alignment, but the NoMultipleSpacesRule also prohibits "Python-style" inline comments, i.e. comments with two spaces in front:

val mu = 3  // Mean
val sigma = 2  // Variance

I think that adding two spaces in front more clearly separates the comment from the code, and it is not against the Kotlin style guide as far as I know.

Would it be possible to add an option to disable this rule only for spaces around comments?

@paul-dingemans
Copy link
Collaborator

Would it be possible to add an option to disable this rule only for spaces around comments?

Ktlint will not support flags for 'small' details like this. I do understand that it might not be a 'small' details for you but we really should try to avoid to bloat ktlint with tens of configuration issues.

@paul-dingemans paul-dingemans closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2022
@FWDekker
Copy link
Contributor

FWDekker commented Nov 24, 2022

@paul-dingemans Fair, I understand that. Adding many configuration options creates a combinatorial explosion in terms of maintenance and testing.

So, instead, what do you think of bringing Ktlint in line with the Kotlin Style Guide and Android Kotlin Style Guide by allowing additional whitespace before comments, without a corresponding configuration option? That is, make the behaviour suggested in this issue the default without the option to change it, since it's more in line with those major style guides.

@paul-dingemans
Copy link
Collaborator

As was pointed out before:

Under https://kotlinlang.org/docs/reference/coding-conventions.html#horizontal-whitespace,

As a general rule, avoid horizontal alignment of any kind. Renaming an identifier to a name with a different length should not affect the formatting of either the declaration or any of the usages.

I have seen no convincing argument to allow additional spacing before comment that do not violate the general rule 'avoid horizontal alignment of any kind'. Neither is this requested widely by many users.

@FWDekker
Copy link
Contributor

I see what you mean, as some comments did ask for allowing horizontal alignment, even though that is strictly against the style guides. I agree with you here that multiple spaces before a comment with the goal of aligning multiple comments should be disallowed.

The issue raised by OP, however, was slightly different from some of the comments:

  1. Ktlint allowed alignment of consecutive lines, but not for non-consecutive lines, as shown in OP's sample code. (This was fixed in 696b33e in response to this issue, by always disallowing more than one space before a comment.)
  2. Ktlint does not allow a comment to be preceded by two spaces, à la Python, even if there is no alignment going on. (This is still the case.)

Both style guides allow the latter, so I think it would make sense for Ktlint to allow it as well. I don't really see any use cases beyond Python-style comments, so what do you think of changing NoMultipleSpacesRule to tolerate either 1 or 2, but not more? This would still effectively prevent alignment, except in some exceptional cases, with only a minor change in code.

@paul-dingemans
Copy link
Collaborator

I do understand from your perspective that allowing 1 or either 2 spaces before an EOL does not violate the style guide when it is not done for alignment purposes. Although the code change will be fairly simple, I still against it for following reasons:

  • KtLint has a strong focus on consistent code formatting. Allowing 1 or 2 spaces could lead to inconsitent formatting as it would depend on the developer to consistently use 2 space. Of course you can argue that this could be forced by ktlint as well if there was a rule for that. But as explained before, we do not want to add configuration settings for minor details.
  • I expect that additional edge cases will popup. Consider for example a line containing only some indentation plus and EOL-comment.
  • Kotlin != Python, with a different language also come different 'best' practices
  • Allowing 1 or 2 spaces will lead to new issues in which will be complained about not removing a redundant space.

If it is really important for you to have this functionality, I would suggest that you create a custom ruleset that allows/enforces the double space before the EOL comment and disable the rule which is provided by KtLint.

@FWDekker
Copy link
Contributor

Thank you for the response. I understand and completely agree with you.

I might consider writing a custom ruleset that enforces a double space before comments if I find the time to do that, and release it separately.

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

No branches or pull requests

9 participants