nolintlint: enforce directive and linter list formatting

[796RCP92VZ]

The rationale for this PR is to provide more helpful error messages in the nolintlint linter for nolint directives that have leading whitespace. The syntax of a Go directive should roughly conform to the following format: ^[a-z]+:[a-z]+. For nolint directive comments that have leading whitespace, the nolintlint linter currently gives a warning of the form “directive // nolint: lll // ignore line length warnings should be written without leading space as //nolint: lll // ignore line length warnings", whereas the correct directive comment would actually be //nolint:lll // ignore line length warnings (with no space between nolint: and lll). Broadly, this PR avoids providing inaccurate suggestions such as the aforementioned, instead providing a more generic directive template, resulting in an error message such as, "directive // nolint: lll // ignore line length warnings should match //nolint[:<comma-separated-linters>] [// <explanation>]". This should reduce confusion for developers and should make it easier to conform to the requirements of go fmt as of go1.19. However, this PR is non-breaking - it does not result in an error for nolint directives that have no leading whitespace but that do have spaces in the linter lists, which follows the existing behavior of nolintlint.

Change list:

• Remove directiveWithOptionalLeadingSpace field - leading spaces in the directive are no longer allowed as of go1.19, so the directive will now always be //nolint. It cannot be any directive other than nolint (e.g., go:generate) due to the guarantee provided by matching the comment against the commentPattern regular expression in the Run function. Therefore, we can just hardcode //nolint as the directive. (The directive, as the term is generally used in Go, could be something like //nolint:lll or //nolint:all with a colon and specific linter(s) after nolint, but the directiveWithOptionalLeadingSpace variable would not currently include the optional colon and anything following it, so for the purposes of this field, the directive will always be //nolint).
• Update the fullDirectivePattern to disallow spaces at the beginning of the string in order to avoid providing an error message that could potentially contain an inaccurate suggestion for how to update the nolint directive, as would have previously been done by the NotMachine issue. The regular expression continues to allow empty explanation comments because they will be caught by the NoExplanation issue if the NeedsExplanation flag is set.
• Deprecate NeedsMachineOnly and remove all references in code.
• Remove the NotMachine issue, since it has been superseded by the ParseError, which gives a more accurate error message (specifically, it does not provide a suggestion for how to update the nolint directive that could potentially still have spaces in the linter list, but rather a more generic suggestion of the format of a well-formed nolint comment).
• Update NoExplanation details to provide a generic directive template rather than a specific suggestion; update unit tests accordingly. Similar to the previous bullet point, this avoids inadvertently providing an inaccurate error message.
• Remove unused //nolint:gocritic comments.
• Update test cases in the unit test file that previously resulted in the NotMachine issue to reflect that they now result in the ParseError issue instead.
• Add test cases that would have previously erroneously resulted in directiveWithOptionalLeadingSpace values not equal to //nolint or // nolint (namely, directiveWithOptionalLeadingSpace values containing additional characters after the directive, such as //nolint / description).

[ldez]

FYI, the "lax" formatting is intentional not to break existing usage.

Before doing this kind of PR it's important to open an issue to talk about.

[796RCP92VZ]

Sorry, I'm new to the whole open source thing, so I wasn't aware of the standard around this. I did take a brief look at the "Contributing" section on the website before creating this PR, and while it definitely helped me get started setting up the environment and following the PR creation process, it didn't really suggest anywhere that I should consider opening a GitHub issue before contributing or provide guidelines around when it's appropriate to contribute / how to contribute effectively. It could be worth mentioning on that section of the website that developers should consider opening a GitHub issue / viewing the existing issues before contributing, though I would understand if you choose not to include that info on the website if it is considered enough of an industry standard / common knowledge in the open source world.

I hope this PR can still be a useful starting point, and I'm happy to modify the scope of the PR or do some refactoring to better meet the needs and circumstances of this project.

[ldez]

The problem is that your PR is breaking, this is why I said that opening an issue before is important. It's not related to a general workflow.

As I said, your PR is breaking, and it's a problem: we cannot follow this way for now.

Sadly, either you change your PR to be non-breaking or we will not be able to accept it.

[796RCP92VZ]

Could you clarify what it's breaking exactly? Do you mean that it's causing automated test cases to fail, that it produces warnings for a class of nolint comments for which it did not produce warnings before, that it unexports the BaseIssue struct, something else, or some combination of those things?

Would it be considered non-breaking if I were to make the ParseError issue more liberal to allow spaces in the linter list and allow an arbitrary number of spaces thereafter, but still kept the changes to the error messages and issue types (namely, replacing NotMachine with the broader ParseError and changing the error message for NoExplanation to be more generic)? In such an implementation, the exact same sets of nolint comments would pass/fail the nolintlint check as the sets that passed/failed before this change, but some nolint comments would be classified as a ParseError issue instead of a NotMachine issue, and some of the error messages would be different.

[ldez]

If an existing unit test fails with your PR (not just because of a different message), it's breaking, if an existing integration test fails with your PR, it's breaking, if a previous working behavior doesn't work anymore, it's breaking, etc.

FYI, the current failure of the CI is not related to your PR, you should rebase your PR on the branch master. (please don't create a new PR for that #4543)

[796RCP92VZ]

I have updated the PR to be non-breaking via the strategy I described in my previous comment. I have also updated the PR description and the change list accordingly, and rebased off master. Please review my updated PR and let me know if it will be acceptable.

Edit: I just saw that there is a failing test case; I will address this momentarily and then provide an update when it is ready for review.

[ldez]

Technically your PR cannot be non-breaking because you changed the behavior with spaces.

I decline your PR.

In the future, if an issue is assigned to someone it's better to not work on it.
Also, the breaking behavior should be discussed first.

[796RCP92VZ]

Thanks for your reply. I opened an issue here because I did not see any existing issues that specifically discuss the issue I attempted to fix here, just issues that were somewhat related but not the same.