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

Refactor nolintlint handling: Enforce directive and linter list formatting #4543

Closed
wants to merge 7 commits into from
Closed

Refactor nolintlint handling: Enforce directive and linter list formatting #4543

wants to merge 7 commits into from

Conversation

796RCP92VZ
Copy link

The rationale for this PR is to enforce the formatting of the spacing in the nolint directive via the nolintlint linter. 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", when it should actually suggest //nolint:lll // ignore line length warnings (with no space between nolint: and lll) instead. Broadly, this PR makes the nolintlint warnings more strictly conform to the syntax of a Go directive and avoids providing inaccurate suggestions such as the aforementioned. This should reduce confusion for developers and should make it easier to conform to the requirements of go fmt as of go1.19.

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 in the linter list. The updated regular expression also disallows 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.
  • Unexport BaseIssue struct, since it should not be used outside nolintlint.go.
  • Deprecate NeedsMachineOnly and remove all references in code.
  • Remove the NotMachine issue, since it has been superseded by the ParserError, 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 should be marked as invalid due to whitespace before/between/after the list of linters.
  • Update test cases 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).

Copy link

boring-cyborg bot commented Mar 19, 2024

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Aaron Pradhan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@796RCP92VZ 796RCP92VZ closed this Mar 19, 2024
@796RCP92VZ 796RCP92VZ deleted the feat/refactor-nolintlint-directive-format branch March 19, 2024 21:50
@ldez ldez added the declined label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants