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

Normalize exclude-rules paths for Windows #2387

Merged
merged 1 commit into from
Aug 24, 2022

Conversation

arjenvanderende
Copy link
Contributor

@arjenvanderende arjenvanderende commented Nov 30, 2021

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

@boring-cyborg
Copy link

boring-cyborg bot commented Nov 30, 2021

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Nov 30, 2021

CLA assistant check
All committers have signed the CLA.

@ldez ldez added the enhancement New feature or improvement label Nov 30, 2021
@ldez ldez self-requested a review November 30, 2021 11:23
@ldez ldez added blocked Need's direct action from maintainer platform: windows Issue that is related to Windows labels Nov 30, 2021
@ldez
Copy link
Member

ldez commented Mar 31, 2022

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.

@kohenkatz
Copy link

@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?

kamilsk
kamilsk previously approved these changes Jul 22, 2022
@ldez ldez dismissed kamilsk’s stale review July 22, 2022 13:52

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

@kamilsk
Copy link
Contributor

kamilsk commented Jul 22, 2022

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.

@ldez ldez removed the blocked Need's direct action from maintainer label Jul 30, 2022
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The following PR will fix the CI for Windows #3134

@ldez ldez merged commit 890a826 into golangci:master Aug 24, 2022
SeigeC pushed a commit to SeigeC/golangci-lint that referenced this pull request Apr 4, 2023
@ldez ldez added this to the v1.50 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement platform: windows Issue that is related to Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalize path in exclude rules (OS path separator)
5 participants