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

Deprecate Interfacer linter #1755

Merged
merged 3 commits into from Feb 21, 2021
Merged

Deprecate Interfacer linter #1755

merged 3 commits into from Feb 21, 2021

Conversation

SVilgelm
Copy link
Member

@SVilgelm SVilgelm commented Feb 20, 2021

Add Deprecated function to config that accepts a message as a deprecation reason.
Print a warning if a deprecated linter is enabled.

Deprecate the Interfacer linter due to archived repository.

Related to #541

Add `Deprecated` function to config that accepts a message as a deprecation reason.
Print a warning if a deprecated linter is enabled.

Deprecate the Interfacer linter due to archived repository.
@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

output is:

~/icloud/projects/golangci-lint/golangci-lint run ./...
WARN [runner] The linter 'interfacer' is deprecated due to: The repository of the linter has been archived by the owner. 

@ldez ldez added the linter: update version Update version of linter label Feb 20, 2021
@ldez
Copy link
Member

ldez commented Feb 20, 2021

level=warning msg="[runner] The linter 'interfacer' is deprecated due to: The repository of the linter has been archived by the owner."

The current test system will not work with a warning log.
At least we have to disable interfacer from our .golangci.yml

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

The current test system will not work with a warning log.
At least we have to disable interfacer from our .golangci.yml

Yes, I think how can we ignore only the deprecated messages in tests

@ldez
Copy link
Member

ldez commented Feb 20, 2021

I think we can use log.Infof instead of log.Warnf

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

but then it will not be printed, because the Info level is ignored by default

@ldez
Copy link
Member

ldez commented Feb 20, 2021

maybe we can add an option like --fast to skip the warning. 🤔

@ldez
Copy link
Member

ldez commented Feb 20, 2021

Maybe we can reuse Config.InternalTest 🤔

@ldez
Copy link
Member

ldez commented Feb 20, 2021

I have some good results here: ldez@270a1db

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

cool, please commit it here

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

because I came up only with this:

diff --git a/pkg/commands/run.go b/pkg/commands/run.go
index 357e274..9c611a3 100644
--- a/pkg/commands/run.go
+++ b/pkg/commands/run.go
@@ -6,6 +6,7 @@ import (
        "io/ioutil"
        "log"
        "os"
+       "regexp"
        "runtime"
        "strings"
        "time"
@@ -483,8 +484,19 @@ func (e *Executor) setupExitCode(ctx context.Context) {
                return
        }
 
-       needFailOnWarnings := (os.Getenv("GL_TEST_RUN") == "1" || os.Getenv("FAIL_ON_WARNINGS") == "1")
-       if needFailOnWarnings && len(e.reportData.Warnings) != 0 {
+       glTestRun := os.Getenv("GL_TEST_RUN") == "1"
+       needFailOnWarnings := (glTestRun || os.Getenv("FAIL_ON_WARNINGS") == "1")
+       warnings := len(e.reportData.Warnings)
+       if glTestRun {
+               re := regexp.MustCompile("The linter '.+' is deprecated due to: .+")
+               for _, w := range e.reportData.Warnings {
+                       if re.MatchString(w.Text) {
+                               warnings--
+                       }
+               }
+       }
+
+       if needFailOnWarnings && warnings != 0 {
                e.exitCode = exitcodes.WarningInTest
                return
        }

@ldez
Copy link
Member

ldez commented Feb 20, 2021

Your approach seems also good maybe a bit harder to maintain 🤔

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

current output:

21-02-20 16:53 % ~/icloud/projects/golangci-lint/golangci-lint run ./...                   
WARN [runner] The linter 'interfacer' is deprecated due to: The repository of the linter has been archived by the owner. 
21-02-20 16:53 % echo $?
0

21-02-20 16:53 % FAIL_ON_WARNINGS=1 ~/icloud/projects/golangci-lint/golangci-lint run ./...
WARN [runner] The linter 'interfacer' is deprecated due to: The repository of the linter has been archived by the owner. 
21-02-20 16:53 % echo $?                                                                   
2

@ldez
Copy link
Member

ldez commented Feb 20, 2021

with my approach:

$ ./golangci-lint run 
WARN [runner] The linter 'interfacer' is deprecated due to: The repository of the linter has been archived by the owner.
$ echo $?
2
$ ./golangci-lint run --internal-cmd-test
$ echo $?
0

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

I tested also with yours approach

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

with my approach we will need to maintain same string in two places

@ldez
Copy link
Member

ldez commented Feb 20, 2021

yes, the base message must sync between 2 places.

A variation of your approach can be to apply the filtering in the logger:

func (lw LogWrapper) Warnf(format string, args ...interface{}) {
	lw.origLog.Warnf(format, args...)
	w := Warning{
		Tag:  strings.Join(lw.tags, "/"),
		Text: fmt.Sprintf(format, args...),
	}

+	re := regexp.MustCompile("The linter '.+' is deprecated due to: .+")
+	if re.MatchString(w.Text) {
+		return
+	}

	lw.rd.Warnings = append(lw.rd.Warnings, w)
}

@ldez
Copy link
Member

ldez commented Feb 20, 2021

the advantages of my approach:

  • it can be reuse and extend
  • not specific to logs
  • easy to use in all our tests

@ldez
Copy link
Member

ldez commented Feb 20, 2021

A variation of my approach can be to create a flag like --skip-deprecated-linter-warn but I think it's useless because users can just remove the linter instead of playing with the deprecation.

@SVilgelm
Copy link
Member Author

SVilgelm commented Feb 20, 2021

@ldez approve it?

@ldez
Copy link
Member

ldez commented Feb 20, 2021

ldez
ldez approved these changes Feb 21, 2021
Copy link
Member

@ldez ldez left a comment

LGTM

svengreb added a commit to svengreb/tmpl-go that referenced this pull request Apr 26, 2021
The currently latest `golangci-lint` version 1.39.0 [1] introduced new
linters and updated supported ones:

1. predeclared [2] (v1.35.0 [3]) - Checks for definitions and
   declarations that shadows one of Go's pre-declared identifiers [4].
   This linter is disabled by default, but is enabled for this template
   to help to prevent shadowed identifiers.
2. interfacer [5] (v1.38.0 [6]) - Has been deprecated [7] and removed
   from this template.
3. scopelint [8] (v1.39.0 [9]) - Has been deprecated [10] and replaced
   by exportloopref [11].
   The `exportloopref` linter is disabled by default, but is enabled for
   this template to help to catch loop variable bugs.

[1]: https://github.com/golangci/golangci-lint/releases/tag/v1.39.0
[2]: https://github.com/nishanths/predeclared
[3]: https://github.com/golangci/golangci-lint/releases/tag/v1.35.0
[4]: https://golang.org/ref/spec#Predeclared_identifiers
[5]: https://github.com/mvdan/interfacer
[6]: https://github.com/golangci/golangci-lint/releases/tag/v1.38.0
[7]: golangci/golangci-lint#1755
[8]: https://github.com/kyoh86/scopelint
[9]: https://github.com/golangci/golangci-lint/releases/tag/v1.39.0
[10]: golangci/golangci-lint#1819
[11]: https://github.com/kyoh86/exportloopref

GH-56
svengreb added a commit to svengreb/tmpl-go that referenced this pull request Apr 26, 2021
The currently latest `golangci-lint` version 1.39.0 [1] introduced new
linters and updated supported ones:

1. predeclared [2] (v1.35.0 [3]) - Checks for definitions and
   declarations that shadows one of Go's pre-declared identifiers [4].
   This linter is disabled by default, but is enabled for this template
   to help to prevent shadowed identifiers.
2. interfacer [5] (v1.38.0 [6]) - Has been deprecated [7] and removed
   from this template.
3. scopelint [8] (v1.39.0 [9]) - Has been deprecated [10] and replaced
   by exportloopref [11].
   The `exportloopref` linter is disabled by default, but is enabled for
   this template to help to catch loop variable bugs.

[1]: https://github.com/golangci/golangci-lint/releases/tag/v1.39.0
[2]: https://github.com/nishanths/predeclared
[3]: https://github.com/golangci/golangci-lint/releases/tag/v1.35.0
[4]: https://golang.org/ref/spec#Predeclared_identifiers
[5]: https://github.com/mvdan/interfacer
[6]: https://github.com/golangci/golangci-lint/releases/tag/v1.38.0
[7]: golangci/golangci-lint#1755
[8]: https://github.com/kyoh86/scopelint
[9]: https://github.com/golangci/golangci-lint/releases/tag/v1.39.0
[10]: golangci/golangci-lint#1819
[11]: https://github.com/kyoh86/exportloopref

Closes GH-56
dannykopping added a commit to dannykopping/loki that referenced this pull request Sep 19, 2021
Replacing deprecated golint with revive (golangci/golangci-lint#1965)

Removing interfacer (golangci/golangci-lint#1755)

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
dannykopping added a commit to grafana/loki that referenced this pull request Sep 21, 2021
* Fixing CI deprecations

Replacing deprecated golint with revive (golangci/golangci-lint#1965)

Removing interfacer (golangci/golangci-lint#1755)

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>

* Further suggestions from Marco in #1002

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>

* Clarify instruction

Signed-off-by: Danny Kopping <danny.kopping@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: update version Update version of linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants