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

SwiftLint doesn't find a nested configuration when one specific file is specified #3341

Closed
2 tasks done
sethfri opened this issue Sep 15, 2020 · 3 comments · Fixed by #3342 or #3379
Closed
2 tasks done

SwiftLint doesn't find a nested configuration when one specific file is specified #3341

sethfri opened this issue Sep 15, 2020 · 3 comments · Fixed by #3342 or #3379

Comments

@sethfri
Copy link
Contributor

sethfri commented Sep 15, 2020

New Issue Checklist

Describe the bug

Assume a file exists called Path/To/My/File.swift, and assume Path/To/My is a directory that contains a .swiftlint.yml file. When you tell SwiftLint to lint only the file, it uses the default configuration instead of the YAML file in Path/To/My.

I believe the root cause is due to these lines, which return the default configuration immediately instead of checking the directory for any configurations.

Complete output when running SwiftLint, including the stack trace and command used
$ swiftlint lint -- Path/To/My/File.swift

Environment

  • SwiftLint version (run swiftlint version to be sure)? 0.40.2
  • Installation method used (Homebrew, CocoaPods, building from source, etc)? Homebrew
  • Paste your configuration file:
disabled_rules:
    - cyclomatic_complexity
    - force_cast
    - function_body_length
  • Are you using nested configurations?
    Yes, see above.
  • Which Xcode version are you using (check xcodebuild -version)?
    Xcode 11.7
    Build version 11E801a
  • Do you have a sample that shows the issue? Run echo "[string here]" | swiftlint lint --no-cache --use-stdin --enable-all-rules
    to quickly test if your example is really demonstrating the issue. If your example is more
    complex, you can use swiftlint lint --path [file here] --no-cache --enable-all-rules.
    See above.
// This triggers a violation:
let foo = "Test" as! Array
@sethfri
Copy link
Contributor Author

sethfri commented Sep 17, 2020

I was incorrect about my hypothesized root cause in the initial description. There were two problems that needed fixing here:

  1. When SwiftLint searches for nested configurations and only one file has been passed in via the paths argument, SwiftLint returns early by inadvertently comparing the containing directory of the file to be linted to itself. This happens because rootDirectory is calculated from rootPath, which is set to the file being linted when there is only one file passed into paths
  2. Even if you fix the early return, SwiftLint will merge the configuration file at the root of the file tree (the configurationPath) with the first configuration it finds starting from the leaf where path is (also confusingly called the rootDirectory). It then returns without doing any other merges. SwiftLint does not seem to handle cascading configurations at multiple levels of a repo.

I've got a fix for this in place, just need to clean it up. Will update #3342 when it's ready

@sethfri
Copy link
Contributor Author

sethfri commented Sep 18, 2020

SwiftLint does not seem to handle cascading configurations at multiple levels of a repo

I was mistaken; this actually isn't SwiftLint's intended behavior. This is stated clearly in the README, I just missed it. So (2) above doesn't need to be done, we just need to fix (1). Updating the PR now.

sethfri added a commit to sethfri/SwiftLint that referenced this issue Sep 18, 2020
sethfri added a commit to sethfri/SwiftLint that referenced this issue Sep 20, 2020
jpsim pushed a commit that referenced this issue Sep 22, 2020
Fixes #3341

When SwiftLint searches for nested configurations and only one file has been passed in via the `paths` argument, SwiftLint returns early after inadvertently comparing the containing directory of the file-to-be-linted to itself. This happens because `rootDirectory` is calculated from `rootPath`, which is set to the file being linted in this scenario.
@sethfri
Copy link
Contributor Author

sethfri commented Sep 25, 2020

@jpsim Feels like we should open this one back up since the change had to be reverted. I don't seem to have permissions to do that if you wouldn't mind!

@jpsim jpsim reopened this Sep 25, 2020
jpsim pushed a commit that referenced this issue Nov 8, 2020
This was previously attempted in #3342, but produced a bug in the case where `--config` is used to specify a config from outside of the source tree. The `--config` argument wasn't always being used as an override, and was being merged with the config in the source tree. This has now been addressed and reverts the revert done in #3362. 

Fixes #3341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants