tphakala / birdnet-go

Realtime BirdNET soundscape analyzer
Other
137 stars 14 forks source link

Fix all golangci-lint issues #112

Closed isZumpo closed 2 months ago

isZumpo commented 2 months ago

I would like to see all linting issues resolved such that the linting gate can actually be useful. I could probably go through and fix most of them quite quickly. However, I wonder which strategy should be used to deal with "unused" code such as variables and in some cases empty branches.

Unusued variables For the unused variables etc, should we just go ahead and remove them? Or should we comment them out? I would prefer removing them since we can always go back in git history and bring them back if desired, while commenting bloats the code...

Empty branches From a very quick look, many of these appears to be commented out logging. For example this file in internal/analysis/realtime.go:

func closeDataStore(store datastore.Interface) {
    if err := store.Close(); err != nil {
        //logger.Error("main", "Failed to close database: %v", err)
    } else {
        //logger.Info("main", "Successfully closed database")
    }
}

Do we want to enable the logging again?

These are currently all linting issues:

internal/httpcontroller/init.go:63:4: ineffectual assignment to err (ineffassign)
                        err = s.Echo.StartAutoTLS(":" + settings.WebServer.Port)
                        ^
root@601846a86cb9:/workspaces/BirdNET-Go# golangci-lint run       
internal/myaudio/buffers.go:35:18: Error return value of `ringBuffer.Write` is not checked (errcheck)
        ringBuffer.Write(data)
                        ^
internal/myaudio/buffers.go:85:16: Error return value is not checked (errcheck)
                                ProcessData(bn, data, startTime)
                                           ^
internal/myaudio/capture.go:53:23: Error return value of `malgoCtx.Uninit` is not checked (errcheck)
        defer malgoCtx.Uninit()
                             ^
internal/myaudio/capture.go:135:19: Error return value of `device.Stop` is not checked (errcheck)
        defer device.Stop()
                         ^
cmd/root.go:29:12: Error return value is not checked (errcheck)
        setupFlags(rootCmd, settings)
                  ^
internal/birdnet/rangefilter.go:230:6: func `mergeSpeciesLists` is unused (unused)
func mergeSpeciesLists(list1, list2 []string) []string {
     ^
internal/analysis/processor/actions.go:37:2: field `pcmData` is unused (unused)
        pcmData      []byte
        ^
internal/analysis/processor/execute.go:18:5: var `speciesActionsMap` is unused (unused)
var speciesActionsMap map[string]SpeciesActionConfig
    ^
internal/analysis/processor/processor.go:38:2: field `pcmDataExt` is unused (unused)
        pcmDataExt []byte
        ^
internal/httpcontroller/init.go:93:2: S1000: should use for range instead of for { select {} } (gosimple)
        for {
        ^
internal/myaudio/process.go:63:2: S1021: should merge variable declaration with assignment on next line (gosimple)
        var elapsedTime time.Duration
        ^
internal/mqtt/mqtt.go:54:5: S1002: should omit comparison to bool constant, can be simplified to `!c.internalClient.IsConnected()` (gosimple)
        if c.internalClient.IsConnected() == false {
           ^
internal/observation/observation.go:97:11: printf: fmt.Sprintf format %.1f has arg note.BeginTime of wrong type time.Time (govet)
                line := fmt.Sprintf("%d\tSpectrogram 1\t1\t%s\t%.1f\t%.1f\t0\t15000\t%s\t%s\t%.4f\n",
                        ^
internal/observation/observation.go:157:11: printf: fmt.Sprintf format %f has arg note.BeginTime of wrong type time.Time (govet)
                line := fmt.Sprintf("%f,%f,%s,%s,%.4f\n",
                        ^
internal/httpcontroller/init.go:63:4: ineffectual assignment to err (ineffassign)
                        err = s.Echo.StartAutoTLS(":" + settings.WebServer.Port)
                        ^
internal/analysis/processor/processor.go:199:4: SA9003: empty branch (staticcheck)
                        if p.Settings.Debug {
                        ^
internal/analysis/realtime.go:138:9: SA9003: empty branch (staticcheck)
        } else {
               ^
internal/analysis/realtime.go:136:2: SA9003: empty branch (staticcheck)
        if err := store.Close(); err != nil {
tphakala commented 2 months ago

I agree that linter should not produce errors, I just fixed all of them for now.

I replaced this "//logger.Info("main", "Successfully closed database")" with standard log.Printf() and enabled prints again. However I am not very happy with current logging methods, I would like to have multiple logging levels and easy way to toggle them. I need to overhaul this at some point.

isZumpo commented 2 months ago

Oh very nice! I see that the gate is sucessfull now. Then I will go ahead and close this issue.

Yes, I agree. Multiple levels of loggning would be great. Then could log most things without polluting the logs.