golangci-lint icon indicating copy to clipboard operation
golangci-lint copied to clipboard

Normalize exclude-rules paths for Windows

Open arjenvanderende opened this issue 4 years ago • 5 comments

Minor change to normalize the paths in exclude-rules. This re-uses the same logic that is already in place for skip-dirs.

Fixes #1398

arjenvanderende avatar Nov 30 '21 10:11 arjenvanderende

Hey, thank you for opening your first Pull Request !

boring-cyborg[bot] avatar Nov 30 '21 10:11 boring-cyborg[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 30 '21 10:11 CLAassistant

I blocked this PR because I think it will create regressions and side effects. I'm not using Windows so I'm not quickly able to validate the behavior.

ldez avatar Mar 31 '22 21:03 ldez

@ldez Can you elaborate on what you think this will break? As a Windows user, it looks correct to me, but I'd like to see if there's anything specific that you think should be tested. This code is already used for skip-files and skip-dirs, so why would it not work for exclude-rules.path?

kohenkatz avatar Jul 21 '22 02:07 kohenkatz

When a PR is blocked, put an approved without any comment is weird.

OK, got it. normalizePathInRegex is already used by SkipDirs and SkipFiles, and

This re-uses the same logic that is already in place for skip-dirs

sounds safe and valid.

Also, separatorToReplace = regexp.QuoteMeta(string(filepath.Separator)) looks OK for Windows

package main

import (
	"regexp"
	"strings"
)

func main() {
	path := '\\'
	separatorToReplace := regexp.QuoteMeta(string(path))

	file := "some/.*/file.go"
	file = strings.ReplaceAll(file, "/", separatorToReplace)
	re := regexp.MustCompile(file)

	if !re.MatchString("some\\path\\to\\file.go") {
		panic("wrong")
	}
}

on Windows I see paths in format pkg\lint\lintersdb\manager.go, see https://github.com/golangci/golangci-lint/runs/7469347948?check_suite_focus=true

So, I think there are no risks, but it will be great if some windows-specific tests will be added here.

kamilsk avatar Jul 22 '22 15:07 kamilsk