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

🌱 enable nolintlint linter and fix violations #3650

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

spencerschrock
Copy link
Member

What kind of change does this PR introduce?

linter change

What is the current behavior?

  • The various //nolint directives aren't formatted properly and end up in doc strings. This syntax is from the docs here https://go.dev/doc/comment

Directive comments such as //go:generate are not considered part of a doc comment and are omitted from rendered documentation. Gofmt moves directive comments to the end of the doc comment, preceded by a blank line. For example:

package regexp

// An Op is a single regular expression operator.
//
//go:generate stringer -type Op -trimprefix Op
type Op uint8

A directive comment is a line matching the regular expression //(line |extern |export |[a-z0-9]+:[a-z0-9]). Tools that define their own directives should use the form //toolname:directive.

  • Some //nolint directives are too broad and are hiding bugs.

What is the new behavior (if this is a feature change)?**

  • The nolintlint linter is enabled to enforce directive comments

    • //nolint directives without a sublinter are prohibited. They should all include specific linter(s) so we don't hide other bugs. e.g.//nolint:my-linter,some-other-linter.
    • unused //nolint directives are removed
  • two loop aliasing bugs are fixed

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #3588

Special notes for your reviewer

This PR is large sorry. Here are the functional changes I've highlighted from the diff using some greping and trimming

git diff main | grep -E "^[+-]\s" | grep -v "nolint"`

simple alternative

diff --git a/cron/internal/format/mock_doc.go b/cron/internal/format/mock_doc.go
index 782d3fbc..ebcde284 100644
--- a/cron/internal/format/mock_doc.go
+++ b/cron/internal/format/mock_doc.go
@@ -73,2 +73 @@ func (d *mockDoc) GetCheck(name string) (docs.CheckDoc, error) {
-	//nolint: gosimple
-	m, _ := d.checks[name]
+	m := d.checks[name]

simple alternative

diff --git a/options/options_test.go b/options/options_test.go
index 8098e8eb..91e5ed9c 100644
--- a/options/options_test.go
+++ b/options/options_test.go
@@ -113,2 +111 @@ func TestOptions_Validate(t *testing.T) {
-				os.Setenv(EnvVarEnableSarif, "1")
-				defer os.Unsetenv(EnvVarEnableSarif)
+				t.Setenv(EnvVarEnableSarif, "1")

simple alternative

diff --git a/pkg/mock_doc.go b/pkg/mock_doc.go
index 1baa2a84..dd565acf 100644
--- a/pkg/mock_doc.go
+++ b/pkg/mock_doc.go
@@ -73,2 +73 @@ func (d *mockDoc) GetCheck(name string) (docs.CheckDoc, error) {
-	//nolint: gosimple
-	m, _ := d.checks[name]
+	m := d.checks[name]

This one is a loop alias bug you can view here.

G601: Implicit memory aliasing in for loop. (gosec)

diff --git a/pkg/sarif.go b/pkg/sarif.go
index 30f43d01..8ecf9e9d 100644
--- a/pkg/sarif.go
+++ b/pkg/sarif.go
@@ -632,0 +630 @@ func (r *ScorecardResult) AsSARIF(showDetails bool, logLevel log.Level,
+		check := check
@@ -701,0 +700 @@ func (r *ScorecardResult) AsSARIF(showDetails bool, logLevel log.Level,
+				loc := loc

Unused struct

diff --git a/pkg/sarif_test.go b/pkg/sarif_test.go
index 1fa37e52..49d8e719 100644
--- a/pkg/sarif_test.go
+++ b/pkg/sarif_test.go
@@ -106,8 +104,0 @@ func TestSARIFOutput(t *testing.T) {
-	type Check struct {
-		Risk        string   `yaml:"-"`
-		Short       string   `yaml:"short"`
-		Description string   `yaml:"description"`
-		Remediation []string `yaml:"remediation"`
-		Tags        string   `yaml:"tags"`
-	}
-

moved dynamic error to a package level var:

--- a/clients/githubrepo/roundtripper/roundtripper.go
+++ b/clients/githubrepo/roundtripper/roundtripper.go
@@ -20 +20 @@ import (
-	"fmt"
+	"errors"
@@ -39,0 +40,2 @@ const (
+var errGithubCredentials = errors.New("an error occurred while getting GitHub credentials")
+
@@ -63 +65,2 @@ func NewTransport(ctx context.Context, logger *log.Logger) http.RoundTripper {
-		logger.Error(fmt.Errorf("an error occurred while getting GitHub credentials"), "GitHub token env var is not set. Please read https://github.com/ossf/scorecard#authentication")
+		//nolint:lll
+		logger.Error(errGithubCredentials, "GitHub token env var is not set. Please read https://github.com/ossf/scorecard#authentication")

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)


@spencerschrock spencerschrock requested a review from a team as a code owner November 7, 2023 23:42
@spencerschrock spencerschrock requested review from justaugustus and raghavkaul and removed request for a team November 7, 2023 23:42
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #3650 (86f3836) into main (288319a) will decrease coverage by 5.62%.
The diff coverage is 77.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3650      +/-   ##
==========================================
- Coverage   76.12%   70.51%   -5.62%     
==========================================
  Files         205      205              
  Lines       14051    14050       -1     
==========================================
- Hits        10697     9907     -790     
- Misses       2723     3568     +845     
+ Partials      631      575      -56     

@spencerschrock spencerschrock merged commit 92470de into ossf:main Nov 15, 2023
38 checks passed
@spencerschrock spencerschrock deleted the lint/nolintlint branch November 15, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable the nolintlint linter (eventually)
2 participants