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

AllowSeparatedLeadingComment doesn't allow one comment + WS before first statement #104

Closed
mholiday-nyt opened this issue Apr 20, 2021 · 2 comments
Labels
golangci-only Fixed in wsl but not merged to golangci-lint

Comments

@mholiday-nyt
Copy link
Contributor

With this option enabled,

func NewTestApp(t *testing.T, so, se *bytes.Buffer) *App {
	// once we've set the mock we're done in this
	// test process -- even for other unit tests

	keyring.MockInit()

        . . .

produces cmd/test_common.go:15: block should not start with a whitespace

I believe a small change to the logic should fix this. Also, the option isn't documented as the other config options.

        // If we have multiple groups, add support for newline between each group.
        if p.config.AllowSeparatedLeadingComment {
-               if seenCommentGroups > 1 {
-                       allowedLinesBeforeFirstStatement += seenCommentGroups - 1
+               if seenCommentGroups > 0 {
+                       allowedLinesBeforeFirstStatement += seenCommentGroups
                }
        }
@bombsimon
Copy link
Owner

I kinda think this example is correct, it should yield an error. To me, with the option to allow multiple groups I would think this would be the correct way:

Should yield error

BLOCK {
  // Single comment newline

  STMT
}
BLOCK {
  // Multi statmeent

  // Newline but still whitesapce

  STMT
}
BLOCK {

  // Single leading
  STMT
}
BLOCK {

  // Double leading

  // With whitespace
  STMT
}

Should not yield error

BLOCK {
  // Single no leading
  STMT
}
BLOCK {
  // Double no leading

  // whitespace after BLOCK or before STMT
  STMT
}

With your suggested change, we sould only support a single comment group without whitespace after BLOCK or before STMT like any of these.

BLOCK {
  // Single no leading
  STMT
}
BLOCK {
  // Single no leading
  // Still same block
  // Just multiple lines
  STMT
}
BLOCK {
  /*
    Or even this
    I guess?
  */
  STMT
}

Do you agree?

@mholiday-nyt
Copy link
Contributor Author

Well, I think it should allow

BLOCK {
  // comment [possibly multiline]

  STMT
}

The fix I gave works for the example shown above, but doesn't work for the cases in the UT TestWithConfig/allow_separated_leading_comment

The problem is with "add support for newline between each group" where I think it needs to allow
a newline after each group, or perhaps (better), just not allow a newline before the first group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
golangci-only Fixed in wsl but not merged to golangci-lint
Projects
None yet
Development

No branches or pull requests

2 participants