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

Problem matchers: Don't require relative paths to start with ./ or ../ #98

Merged
merged 1 commit into from Dec 17, 2021

Conversation

dominikh
Copy link
Contributor

Errors reported by Go, even when containing relative paths, do not
always begin with ./ or ../ – for example:

$ go build ./...
# honnef.co/go/tools/lintcmd
lintcmd/format.go:28:8: syntax error: cannot use path := filepath.Clean(pos.Filename) as value

We could require either ./, ../ or at least one path separator and
still match Go's output. However, commonly used linters (such as
Staticcheck and golint) never use ./ for relative paths. Their output
stopped being matched when we moved from v1 to v2. I believe that
being able to match the output of linters is worth relaxing the
pattern for.

This change slightly relaxes the stricter pattern that was introduced
as part of v2 to address #46. However, the pattern is still stricter
than it was in v1 and as strict as it can be for most users.

Errors reported by Go, even when containing relative paths, do not
always begin with ./ or ../ – for example:

	$ go build ./...
	# honnef.co/go/tools/lintcmd
	lintcmd/format.go:28:8: syntax error: cannot use path := filepath.Clean(pos.Filename) as value

We could require either ./, ../ or at least one path separator and
still match Go's output. However, commonly used linters (such as
Staticcheck and golint) never use ./ for relative paths. Their output
stopped being matched when we moved from v1 to v2. I believe that
being able to match the output of linters is worth relaxing the
pattern for.

This change slightly relaxes the stricter pattern that was introduced
as part of v2 to address actions#46. However, the pattern is still stricter
than it was in v1 and as strict as it can be for most users.
@dominikh
Copy link
Contributor Author

dominikh commented Feb 3, 2021

Friendly ping

@marten-seemann
Copy link

Not sure who's maintaining this repo. @joshmgross, @thboop or @bryanmacfarlane maybe?
Would it be possible to the this pull request merged some time?

@dominikh
Copy link
Contributor Author

dominikh commented Mar 4, 2021

@marten-seemann In the meantime I can recommend https://github.com/WillAbides/setup-go-faster

@bryanmacfarlane
Copy link
Member

Any idea on how the regex compares to the one VS Code uses? I believe that's where the original one was pulled.

@bryanmacfarlane bryanmacfarlane changed the title Don't require relative paths to start with ./ or ../ Problem matchers: Don't require relative paths to start with ./ or ../ Mar 4, 2021
@bryanmacfarlane
Copy link
Member

related setup-node PR which compared to and pulled from vs code: actions/setup-node#125

@mvdan
Copy link

mvdan commented Mar 18, 2021

@bryanmacfarlane I don't think the author or anyone else in this thread is involved with VS Code. I hope it's clear that, from the original post here, the current regular expression isn't right. I personally don't mind what the final fix ends up being, but we do need a fix sooner than later :)

@dominikh
Copy link
Contributor Author

Exasperated ping.

@bryanmacfarlane
Copy link
Member

@maxim-lobanov can ya'll take a look at this PR?

Copy link
Contributor

@maxim-lobanov maxim-lobanov left a comment

Choose a reason for hiding this comment

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

@dmitry-shibanov @MaksimZhukov could you please proceed with accepting these changes if they look good to you.

@MaksimZhukov MaksimZhukov merged commit fdeec47 into actions:main Dec 17, 2021
panticmilos pushed a commit to panticmilos/setup-go that referenced this pull request Aug 4, 2022
n33pm pushed a commit to n33pm/setup-go that referenced this pull request Oct 17, 2022
adilhusain-s pushed a commit to adilhusain-s/setup-go that referenced this pull request Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants