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

rules: support inverted path match #3617

Merged
merged 4 commits into from
May 31, 2023
Merged

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Feb 19, 2023

It is not possible to use exclude-rules: path to exclude issues for non-test files because it is impossible to write a regexp that reliably matches those (#2440). The new not-path check solves that by inverting the meaning: it excludes if the pattern does not match.

Besides test files, this is also useful for large code bases where some specific code paths have additional constraints. For example, in Kubernetes the usage of certain functions is forbidden in test/e2e/framework but okay elsewhere. Without not-path path-except, a regular expression that carefully matches all other paths has to be used, which is very cumbersome:

- path: ^(api|cluster|cmd|hack|pkg|plugin|staging|test/(cmd|conformance|e2e/(apimachinery|apps|architecture|auth|autoscaling|chaosmonkey|cloud|common|dra|instrumentation|kubectl|lifecycle|network|node|perftype|reporters|scheduling|storage|testing-manifests|upgrades|windows)|e2e_kubeadm|e2e_node|fixtures|fuzz|images|instrumentation|integration|kubemark|list|soak|typecheck|utils)|third_party|vendor)/
  msg: "E2E framework:"
  linters:
  - forbidigo

With path-except, this becomes much simpler:

- path-except: ^test/e2e/framework/
  msg: "E2E framework:"
  linters:
  - forbidigo

Fixes: #2440

@ldez ldez self-requested a review February 19, 2023 19:48
@ldez ldez added the area: config Related to .golangci.yml and/or cli options label Feb 19, 2023
@ldez
Copy link
Member

ldez commented Feb 19, 2023

Hello,

not-path seems really ambiguous as a name.

@ldez ldez added the blocked Need's direct action from maintainer label Feb 19, 2023
@pohly
Copy link
Contributor Author

pohly commented Feb 20, 2023

not-path seems really ambiguous as a name.

Let's brainstorm, perhaps we can come up with something better.

  • issues: exclude-rules: not-path = "exclude, but not in path ..."
  • issues: exclude-rules: except-for-path = "exclude, except when the path matches"
  • issues: exclude-rules: allow-issues-for-path = "exclude issues, allow them for certain paths"

cc @ashanbrown

@ashanbrown
Copy link
Contributor

xpath? inverse-path?

@pohly pohly force-pushed the exclude-not-path branch 2 times, most recently from c05858e to a3643ad Compare February 21, 2023 09:08
@ashanbrown
Copy link
Contributor

Thinking a bit more about this, path-except seems like it might be clearer name. Furthermore, adding both paths (plural) as an inclusive list as well as paths-except as an exclusive list (applied after pathandpaths`), might be ideal.

@pohly
Copy link
Contributor Author

pohly commented Feb 26, 2023

I'm fine with path-except. I'll defer to @ldez here.

Regarding paths and paths-except, do you want those to be lists of regexps instead of the single regexp for path and (soon?) path-except? That doesn't add any new capabilities (a regexp can already match multiple different paths), so I would prefer to not add those.

@pohly
Copy link
Contributor Author

pohly commented Feb 26, 2023

Semantically, all of the conditions are AND and there's no implied ordering. That more complex conditions get checked after others is merely an implementation detail.

@ashanbrown
Copy link
Contributor

My only reason for suggesting paths and paths-except is that they could make it easier to express intent and keep completely unrelated cases separately (and potentially comment them in yaml as well). I agree it isn't technically necessary to implement the desired behavior.

@pohly
Copy link
Contributor Author

pohly commented Feb 26, 2023

I suspect that it is already possible to keep such separate cases also separated in the YAML with different exclude-rules: they might share other common attributes (linter, message), but if the intent is different, then perhaps those are also different?

@ldez ldez added enhancement New feature or improvement and removed blocked Need's direct action from maintainer labels Apr 14, 2023
@ldez
Copy link
Member

ldez commented Apr 21, 2023

path-except seems right.
Keep it as a string.

@ldez ldez added the feedback required Requires additional feedback label Apr 21, 2023
@pohly pohly force-pushed the exclude-not-path branch 2 times, most recently from cc5e128 to bb208bd Compare April 23, 2023 15:46
@pohly
Copy link
Contributor Author

pohly commented Apr 23, 2023

Renamed to path-except.

@pohly pohly force-pushed the exclude-not-path branch 2 times, most recently from 6e5a8fe to 9fc78a6 Compare April 23, 2023 17:36
It is not possible to use `exclude-rules: path` to exclude issues for non-test
files because it is impossible to write a regexp that reliably matches
those (golangci#2440). The new
`path-except` check solves that by inverting the meaning: it excludes if the
pattern does not match.

Besides test files, this is also useful for large code bases where some
specific code paths have additional constraints. For example, in Kubernetes the
usage of certain functions is forbidden in test/e2e/framework but okay elsewhere.
Without `path-except`, a regular expression that carefully matches all other paths
has to be used, which is very cumbersome:

    - path: ^(api|cluster|cmd|hack|pkg|plugin|staging|test/(cmd|conformance|e2e/(apimachinery|apps|architecture|auth|autoscaling|chaosmonkey|cloud|common|dra|instrumentation|kubectl|lifecycle|network|node|perftype|reporters|scheduling|storage|testing-manifests|upgrades|windows)|e2e_kubeadm|e2e_node|fixtures|fuzz|images|instrumentation|integration|kubemark|list|soak|typecheck|utils)|third_party|vendor)/
      msg: "E2E framework:"
      linters:
      - forbidigo

With path-except, this becomes much simpler:

    - path-except: ^test/e2e/framework/
      msg: "E2E framework:"
      linters:
      - forbidigo
@ldez ldez removed the feedback required Requires additional feedback label Apr 23, 2023
@ldez
Copy link
Member

ldez commented May 26, 2023

Sorry for the delay, I will handle the PR very soon.

@ldez ldez merged commit 8fde463 into golangci:master May 31, 2023
16 checks passed
@ldez ldez added this to the v1.53 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: config Related to .golangci.yml and/or cli options enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

proposal: add a way to exclude rules only for non-test code
4 participants