Enable & fix more linters

.golangci.yml

@@ -6,10 +6,10 @@ linters:
   enable:
     - megacheck
     - govet
-#    - goconst
+    - goconst
-#    - gocritic
+    - gocritic
-#    - gocyclo
+    - gocyclo
-#    - goerr113
+    - goerr113
 #    - goheader # TODO: Needs config
     - gofmt
     - goimports
@@ -40,12 +40,18 @@ issues:
         - varcheck
         - unparam
         - bodyclose
+    - path: pkg/integrations/*
+      text: "unlambda"
+      linters:
+        - gocritic
     - path: pkg/modules/background/unsplash/unsplash\.go
       linters:
         - bodyclose
     - path: pkg/migration/*
       linters:
         - exhaustive
+        - goconst
+        - goerr113
     - path: pkg/models/task_collection_filter\.go
       linters:
         - exhaustive
@@ -53,3 +59,13 @@ issues:
       text: "G404:" # We don't care about cryptographically secure randomness when we're using that utility function.
       linters:
         - gosec
+    - path: pkg/modules/dump/*
+      linters:
+        - goerr113
+    - path: pkg/
+      text: "err113: do not define dynamic errors, use wrapped static errors instead:"
+      linters:
+        - goerr113
+    - text: "commentFormatting: put a space between `//` and comment text"
+      linters:
+        - gocritic

go.mod

@@ -33,6 +33,7 @@ require (
 	github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835
 	github.com/gabriel-vasile/mimetype v1.1.1
 	github.com/getsentry/sentry-go v0.7.0
+	github.com/go-errors/errors v1.0.1
 	github.com/go-redis/redis/v7 v7.4.0
 	github.com/go-sql-driver/mysql v1.5.0
 	github.com/go-testfixtures/testfixtures/v3 v3.4.0

pkg/files/error.go

@@ -29,7 +29,7 @@ func (err ErrFileDoesNotExist) Error() string {
 	return fmt.Sprintf("file %d does not exist", err.FileID)
 }
 
-//IsErrFileDoesNotExist checks if an error is ErrFileDoesNotExist
+// IsErrFileDoesNotExist checks if an error is ErrFileDoesNotExist
 func IsErrFileDoesNotExist(err error) bool {
 	_, ok := err.(ErrFileDoesNotExist)
 	return ok
@@ -45,7 +45,7 @@ func (err ErrFileIsTooLarge) Error() string {
 	return fmt.Sprintf("file is too large [Size: %d]", err.Size)
 }
 
-//IsErrFileIsTooLarge checks if an error is ErrFileIsTooLarge
+// IsErrFileIsTooLarge checks if an error is ErrFileIsTooLarge
 func IsErrFileIsTooLarge(err error) bool {
 	_, ok := err.(ErrFileIsTooLarge)
 	return ok
@@ -62,7 +62,7 @@ func (err ErrFileIsNotUnsplashFile) Error() string {
 	return fmt.Sprintf("file was not downloaded from unsplash [FileID: %d]", err.FileID)
 }
 
-//IsErrFileIsNotUnsplashFile checks if an error is ErrFileIsNotUnsplashFile
+// IsErrFileIsNotUnsplashFile checks if an error is ErrFileIsNotUnsplashFile
 func IsErrFileIsNotUnsplashFile(err error) bool {
 	_, ok := err.(ErrFileIsNotUnsplashFile)
 	return ok

pkg/files/filehandling.go

@@ -50,7 +50,7 @@ func initFixtures(t *testing.T) {
 	InitTestFileFixtures(t)
 }
 
-//InitTestFileFixtures initializes file fixtures
+// InitTestFileFixtures initializes file fixtures
 func InitTestFileFixtures(t *testing.T) {
 	// Init fixture files
 	filename := config.FilesBasePath.GetString() + "/1"

pkg/log/logging.go

@@ -106,7 +106,6 @@ func GetLogger() *logging.Logger {
 	return logInstance
 }
 
-/////
 // The following functions are to be used as an "eye-candy", so one can just write log.Error() instead of log.Log.Error()
 
 // Debug is for debug messages

pkg/migration/20200516123847.go

@@ -56,7 +56,7 @@ func init() {
 				// The general idea here is to take the title and slice it into pieces, until we found a unique piece.
 
 				var exists = true
-				titleSlug := []rune(strings.Replace(strings.ToUpper(l.Title), " ", "", -1))
+				titleSlug := []rune(strings.ReplaceAll(strings.ToUpper(l.Title), " ", ""))
 
 				// We can save at most 10 characters in the db, so we need to ensure it has at most 10 characters
 				if len(titleSlug) > 10 {

pkg/migration/20200621214452.go

@@ -655,7 +655,7 @@ create unique index UQE_users_namespace_id
 						// The statement is probably useless anyway since its only purpose is to clean up old tables
 						// which may be leftovers from a previously failed migration. However, since the whole thing
 						// is wrapped in sessions, this is extremely unlikely to happen anyway.
-						//"ALTER TABLE " + table + " DROP COLUMN IF EXISTS " + colTmp + ";",
+						// "ALTER TABLE " + table + " DROP COLUMN IF EXISTS " + colTmp + ";",
 						"ALTER TABLE " + table + " ADD COLUMN " + colTmp + " DATETIME NULL;",
 						// #nosec
 						"UPDATE " + table + " SET " + colTmp + " = IF(" + colOld + " = 0, NULL, FROM_UNIXTIME(" + colOld + "));",

pkg/models/list.go

@@ -451,7 +451,7 @@ func GenerateListIdentifier(l *List, sess *xorm.Engine) (err error) {
 	// The general idea here is to take the title and slice it into pieces, until we found a unique piece.
 
 	var exists = true
-	titleSlug := []rune(strings.Replace(strings.ToUpper(l.Title), " ", "", -1))
+	titleSlug := []rune(strings.ReplaceAll(strings.ToUpper(l.Title), " ", ""))
 
 	// We can save at most 10 characters in the db, so we need to ensure it has at most 10 characters
 	if len(titleSlug) > 10 {

pkg/models/list_duplicate.go

@@ -65,6 +65,7 @@ func (ld *ListDuplicate) CanCreate(a web.Auth) (canCreate bool, err error) {
 // @Failure 403 {object} web.HTTPError "The user does not have access to the list or namespace"
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /lists/{listID}/duplicate [put]
+//nolint:gocyclo
 func (ld *ListDuplicate) Create(a web.Auth) (err error) {
 
 	log.Debugf("Duplicating list %d", ld.ListID)

pkg/models/tasks.go

@@ -169,6 +169,7 @@ func (t *Task) ReadAll(a web.Auth, search string, page int, perPage int) (result
 	return nil, 0, 0, nil
 }
 
+//nolint:gocyclo
 func getRawTasksForLists(lists []*List, a web.Auth, opts *taskOptions) (tasks []*Task, resultCount int, totalItems int64, err error) {
 
 	// If the user does not have any lists, don't try to get any tasks
@@ -731,6 +732,7 @@ func createTask(s *xorm.Session, t *Task, a web.Auth, updateAssignees bool) (err
 // @Failure 403 {object} web.HTTPError "The user does not have access to the task (aka its list)"
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /tasks/{id} [post]
+//nolint:gocyclo
 func (t *Task) Update() (err error) {
 
 	s := x.NewSession()
@@ -839,12 +841,12 @@ func (t *Task) Update() (err error) {
 	// a lot of existing code which we'll then need to refactor.
 	// This is why.
 	//
-	//if err := ot.updateTaskLabels(t.Labels); err != nil {
+	// if err := ot.updateTaskLabels(t.Labels); err != nil {
-	//	return err
+	// 	return err
-	//}
+	// }
 	// set the labels to ot.Labels because our updateTaskLabels function puts the full label objects in it pretty nicely
 	// We also set this here to prevent it being overwritten later on.
-	//t.Labels = ot.Labels
+	// t.Labels = ot.Labels
 
 	// For whatever reason, xorm dont detect if done is updated, so we need to update this every time by hand
 	// Which is why we merge the actual task struct with the one we got from the db

pkg/models/tasks_rights.go

@@ -39,7 +39,6 @@ func (t *Task) CanCreate(a web.Auth) (bool, error) {
 
 // CanRead determines if a user can read a task
 func (t *Task) CanRead(a web.Auth) (canRead bool, maxRight int, err error) {
-	//return t.canDoTask(a)
 	// Get the task, error out if it doesn't exist
 	*t, err = GetTaskByIDSimple(t.ID)
 	if err != nil {

pkg/modules/keyvalue/redis/redis.go

@@ -20,6 +20,8 @@ package redis
 import (
 	"encoding/json"
 
+	"github.com/go-errors/errors"
+
 	e "code.vikunja.io/api/pkg/modules/keyvalue/error"
 	"code.vikunja.io/api/pkg/red"
 	"github.com/go-redis/redis/v7"
@@ -53,7 +55,7 @@ func (s *Storage) Put(key string, value interface{}) (err error) {
 func (s *Storage) Get(key string) (value interface{}, err error) {
 	b, err := s.client.Get(key).Bytes()
 	if err != nil {
-		if err == redis.Nil {
+		if errors.Is(err, redis.Nil) {
 			return nil, &e.ErrValueNotFoundForKey{Key: key}
 		}
 		return nil, err

pkg/modules/migration/todoist/todoist_test.go

@@ -44,7 +44,6 @@ func TestConvertTodoistToVikunja(t *testing.T) {
 	dueTime = dueTime.In(config.GetTimeZone())
 	nilTime, err := time.Parse(time.RFC3339Nano, "0001-01-01T00:00:00Z")
 	assert.NoError(t, err)
-	//nilTime = nilTime.In(config.GetTimeZone())
 	exampleFile, err := ioutil.ReadFile(config.ServiceRootpath.GetString() + "/pkg/modules/migration/wunderlist/testimage.jpg")
 	assert.NoError(t, err)
 

pkg/routes/caldav/listStorageProvider.go

@@ -175,7 +175,7 @@ func (vcls *VikunjaCaldavListStorage) GetResourcesByFilters(rpath string, filter
 	r.Name = vcls.list.Title
 	return []data.Resource{r}, nil
 	// For now, filtering is disabled.
-	//return vcls.GetResources(rpath, false)
+	// return vcls.GetResources(rpath, false)
 }
 
 func getTaskURL(task *models.Task) string {
@@ -337,9 +337,9 @@ func (vlra *VikunjaListResourceAdapter) CalculateEtag() string {
 	// And therefore, updating the task fails since these etags don't match.
 	// To fix that, we use this extra field to determine if we're currently updating a task and return the
 	// etag of the list instead.
-	//if vlra.list != nil {
+	// if vlra.list != nil {
-	//	return `"` + strconv.FormatInt(vlra.list.ID, 10) + `-` + strconv.FormatInt(vlra.list.Updated, 10) + `"`
+	//	 return `"` + strconv.FormatInt(vlra.list.ID, 10) + `-` + strconv.FormatInt(vlra.list.Updated, 10) + `"`
-	//}
+	// }
 
 	// Return the etag of a task if we have one
 	if vlra.task != nil {

pkg/user/totp.go

@@ -36,7 +36,7 @@ type TOTP struct {
 }
 
 // TableName holds the table name for totp secrets
-func (T *TOTP) TableName() string {
+func (t *TOTP) TableName() string {
 	return "totp"
 }
 

pkg/user/user.go

@@ -18,6 +18,7 @@
 package user
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"time"
@@ -190,7 +191,7 @@ func CheckUserCredentials(u *Login) (*User, error) {
 func CheckUserPassword(user *User, password string) error {
 	err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(password))
 	if err != nil {
-		if err == bcrypt.ErrMismatchedHashAndPassword {
+		if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
 			return ErrWrongUsernameOrPassword{}
 		}
 		return err
@@ -323,7 +324,6 @@ func UpdateUser(user *User) (updatedUser *User, err error) {
 
 	// Check if we have at least a username
 	if user.Username == "" {
-		//return User{}, ErrNoUsername{user.ID}
 		user.Username = theUser.Username // Dont change the username if we dont have one
 	} else {
 		// Check if the new username already exists