feat: pass module dir as --path-prefix arg to golangci-lint

[davidjb]

This ensures that the path output reported by golangci-lint is qualified with respect to a given linted file's path within the repo. Without this option, just the filename itself is outputted, leading to ambiguity as to which file is problematic when multiple files have the same name (e.g. main.go).

The implementation creates a generic argument that can be used for any other commands that need the working directory passed to them during iteration.

---

Before:

main.go:10:9: unused-parameter: parameter 'request' seems to be unused, consider removing or renaming it as _ (revive)

After:

modules/compute/main.go:10:9: unused-parameter: parameter 'request' seems to be unused, consider removing or renaming it as _ (revive)

[TekWizely]

Hi @davidjb ! Thanks for taking the time to submit a PR.

I also appreciate you taking the time to try to create a generic solution that blends into the framework.

My concerns on defaulting to an argument like this is whether it is completely benign or not.

The definition of the argument sounds like it is:

.golangci.reference.yml

  # Add a prefix to the output file references.
  # Default is no prefix.
  path-prefix: ""

golangci-lint run -h

--path-prefix string             Path prefix to add to output

BUT this section of the docs suggests it may not be:

False Positives: Exclude Issues by Path

Exclude issues in path by `run.skip-dirs`, `run.skip-files` or `issues.exclude-rules` config options.

Beware that the paths that get matched here are relative to the current working directory.
When the configuration contains path patterns that check for specific directories,
the `--path-prefix` parameter can be used to extend the paths before matching.

Always extend[ing] the paths before matching feels like it could lead to unpredictable behavior for users who aren't expecting it?

It might be worth opening a clarification ticket on the project to better understand what they mean in the documentation.

You can also find code usages of the value here:

• https://github.com/search?q=repo%3Agolangci%2Fgolangci-lint++PathPrefix&type=code

In the meantime, I think it may be nicer to make this into an opt-in action.

Q: Are you familiar with the --hook:* arguments that this projects has support for?

I'm thinking we add support in the golangci-lint hooks for a --hook:path-prefix option that triggers this usage.

Q: Unlike packages which are single folder depth, modules support nested folders, only stopping at folders which themselves have a go.mod file: Have you verified that you get expected results for files that are below the initial mod root folder?

Lemme know your thoughts and thanks again for opening the PR !

-TW

[TekWizely]

@davidjb no rush, just checking in to see if you had a chance to read my post and if you had any thoughts on it ...

[davidjb]

@TekWizely thanks for your comments and review.

Always extend[ing] the paths before matching feels like it could lead to unpredictable behavior for users who aren't expecting it?

This is a tough one. From my perspective, given that pre-commit hooks run and are configured at a repository level and my .golangci.yaml is at the root, you can see why someone in my situation would expect the paths to be relative to the root of the repo. However, as you've rightly highlighted, this isn't the case - the exclude rules are relative to the root the current working directory -- where golangci-lint is being run from.

Given golangci-lint config files can be anywhere from the directory of the analysed file upwards, and paths in each config file aren't relative to that file/dir but actually to the CWD, this is a sticky situation best left up to the linter to figure out.

I'm thinking we add support in the golangci-lint hooks for a --hook:path-prefix option that triggers this usage.

I very much agree. Make it opt-in for those who know what the option does and who can ensure the option will work as expected, and leaves the default behaviour of the linter unchanged.

Q: Are you familiar with the --hook:* arguments that this projects has support for?

I wasn't aware of these hook arguments but I am now - I pushed a change to adjust my PR accordingly to use hook arguments. See what you think - I've done the best I can in the current setup to separate concerns (e.g. avoid putting anything tool-specific into the generic scripts), but this was unavoidable in lib/common.bash. You might want to rename the hook to be more generic - whatever suits.

Q: Unlike packages which are single folder depth, modules support nested folders, only stopping at folders which themselves have a go.mod file: Have you verified that you get expected results for files that are below the initial mod root folder?

Yes, most definitely. I've used this/your hooks in a Go monorepo situation and at least 2 levels of nesting modules it's working fine, both for path prefixing and linting in general.

[TekWizely]

Sorry for the delayed review - I had been grinding on this a bit and I think I have a useful suggest.

Give it a read and lemme know your thoughts !

[TekWizely]

Ok I've been thinking on this for a while now ... the thing that is holding up is the =${sub} ...

I don't like assuming that the target tool will understand arg=val (vs arg val) ...

SOO, I'm wondering if the quick fix is to treat cmd_cwd_arg as a template ... ie.

For golangic-lint, the value would be:

cmd_cwd_arg="--path-prefix={{sub}}"

And our insertion script would know to replace {{sub}} with the value of ${sub} ...

This would make it so we don't have to assume the format of argument+value ...