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

Make --fix work (naively) with --prefix-path #3626

Closed
wants to merge 4 commits into from

Conversation

scop
Copy link
Contributor

@scop scop commented Feb 22, 2023

This builds on @moitias' (Hi! 👋) work in #2293, resolves cropped up conflicts and addresses open review comments #2293 (comment) and #2293 (comment).

Issue

Currently, using --path-prefix together with --fix does not work with any other path prefix than "".

This is easily reproduced merely by adding --path-prefix=something to the command line when also using --fix (and, of course, actually having something fixable) or adding it to the arguments used in TestFix, which will cause the test to fail.

This is problematic for my use case, a repository with multiple go modules in it (in separate directories) where I'd like to run golangci-lint on all of them with pre-commit before commiting. To do this, I did not figure out other approaches than to run golangci-lint in each of those directories and using --path-prefix to add the module directory to the output which does not work.

Reproduction

matias@kiri ~/c/t/fix_sample> ls
main.go
matias@kiri ~/c/t/fix_sample> cat main.go 
package main

func main() { 
	println(  
"hello") 
}
matias@kiri ~/c/t/fix_sample> golangci-lint version
golangci-lint has version 1.42.0 built from c6142e38 on 2021-08-17T11:47:22Z
matias@kiri ~/c/t/fix_sample> golangci-lint run --fix --path-prefix=foo ./main.go -v --disable-all --enable gofmt
INFO [config_reader] Config search paths: [./ /home/matias/code/temp/fix_sample /home/matias/code/temp /home/matias/code /home/matias /home /] 
INFO [lintersdb] Active 1 linters: [gofmt]        
INFO [loader] Go packages loading at mode 7 (compiled_files|files|name) took 41.533512ms 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 122.782µs 
INFO [linters context/goanalysis] analyzers took 731.031µs with top 10 stages: gofmt: 731.031µs 
INFO [runner] Processors filtering stat (out/in): max_same_issues: 1/1, max_from_linter: 1/1, severity-rules: 1/1, path_prefixer: 1/1, sort_results: 1/1, identifier_marker: 1/1, path_prettifier: 1/1, autogenerated_exclude: 1/1, exclude: 1/1, nolint: 1/1, uniq_by_line: 1/1, max_per_file_from_linter: 1/1, path_shortener: 1/1, filename_unadjuster: 1/1, diff: 1/1, skip_files: 1/1, skip_dirs: 1/1, exclude-rules: 1/1, source_code: 1/1, cgo: 1/1 
INFO [runner] processing took 123.9µs with stages: identifier_marker: 22.489µs, exclude-rules: 14.388µs, nolint: 13.41µs, path_prettifier: 12.431µs, autogenerated_exclude: 12.153µs, source_code: 9.429µs, skip_dirs: 5.308µs, cgo: 3.841µs, exclude: 3.352µs, skip_files: 2.934µs, sort_results: 2.864µs, uniq_by_line: 2.793µs, diff: 2.445µs, severity-rules: 2.444µs, path_shortener: 2.375µs, path_prefixer: 2.374µs, max_same_issues: 2.305µs, filename_unadjuster: 2.305µs, max_per_file_from_linter: 2.304µs, max_from_linter: 1.956µs 
INFO [runner] linters took 1.433009ms with stages: gofmt: 1.159161ms 
ERRO Failed to fix issues in file foo/main.go: failed to get file bytes for foo/main.go: can't read file foo/main.go: open foo/main.go: no such file or directory 
INFO fixer took 16.623µs with stages: all: 16.623µs 
foo/main.go:3: File is not `gofmt`-ed with `-s` (gofmt)
func main() { 
	println(  
"hello") 
INFO File cache stats: 1 entries of total size 53B 
INFO Memory: 2 samples, avg is 72.7MB, max is 72.8MB 
INFO Execution took 50.472601ms           

(even though this test was ran with not quite the latest version, the issue reproduces with tip of master as well)

The error in the output clearly points out the problem;

ERRO Failed to fix issues in file foo/main.go: failed to get file bytes for foo/main.go: can't read file foo/main.go: open foo/main.go: no such file or directory

That is, the path prefix is prepended to the path of the file to be fixed which does not seem to be what is intended, as the help / documentation states that --prefix-path configures the Path prefix to add to **output**.

Fix

This PR adds a mock --path-prefix argument to TestFix and implements a (very naive) fix for the issue, stripping the configured path prefix from the path passed to Fixer.fixIssuesInFile if the configured path prefix is not "".

I'm happy to create an alternative fix if some other approach is seen to be better, options off the top of my head would include;

  • Adding the actual path to token.Position, allowing Fixer to use it instead.
  • Adding the actual path to Issue, allowing Issue.FilePath() to return it, as this is how Fixer seems to determine the path to use.
  • Adding the path prefix only to (all of?) the output(s) rather than prefixing token.Position.Filename as there could be other needs for the actual path of the file in the future? This seems like the best option to me but the details of how to do this and implications it could have are unclear to me as I'm quite unfamiliar with the codebase, so would need some pointers to implement this.

@ldez ldez added the enhancement New feature or improvement label Feb 22, 2023
@ldez ldez self-requested a review February 22, 2023 19:24
@ldez ldez added area: auto-fix blocked Need's direct action from maintainer labels Feb 22, 2023
@pohly
Copy link
Contributor

pohly commented Mar 16, 2023

Yet another potential solution is to move the fixer before the path prefixer - see #3700 for that approach.

@ldez
Copy link
Member

ldez commented Mar 20, 2023

The approach in #3700 seems more robust and more coherent with the golangci-lint design.

I will close this PR and merge the other one.

I would thanks @moitias for creating the base PR with a very detailed description.
Thanks to @scop for trying to bring life back to the topic, and the work on this PR.

@ldez ldez closed this Mar 20, 2023
@ldez ldez added declined and removed blocked Need's direct action from maintainer labels Mar 20, 2023
@pohly
Copy link
Contributor

pohly commented Mar 20, 2023

I agree, the description of this PR was very useful - thanks!

@scop scop deleted the fix/fix-with-prefix-path branch August 14, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants