Plugin doesn't respect per-directory configs (nested configuration).

[sunshinejr]

👋 (basic reference Harvey#11)

As per title, we have a setup that does rely on Sources having different config file and Tests having different config file as well (this is described as valid nested configuration setting here). This works correctly on local machine, but on CI with Danger & SwiftLint it just goes with default root config.

I didn't really have time to look this one up in the code, I know that Ash doesn't have much time to explore this one either, so if anyone is up for checking this one out please feel free to do so.

Cheers!

[sunshinejr]

Okay, had some time to investigate it:

Basically I had SwiftLint 0.16~ and there is version 0.23 already. From some 0.2X version there is a "fixed bug" or "regression", depends how you look at it. Basically when you use swiftlint on entire project, and then e.g. want to swiftlint only one file with command:

swiftlint lint --quiet --path Sources/Harvey/Harvey.swift --reporter json

It does report different results if you have multiple .swiftlint.yml configs (see Nested Configuration paragraph). So, the workaround is to go to the directory with that file, and then execute swiftlint:

cd Sources/Harvey
swiftlint lint --quiet --path Harvey.swift --reporter json

Will try to make a PR with this change and we can discuss it further there.

[ashfurrow]

Hmm, very strange. Any issues/docs/discussions you found that would add more context?

[ashfurrow]

And thanks for looking into this!

[sunshinejr]

@ashfurrow Yeah, forgot to add related issues, sorry about that! At least these two SwiftLint#1745 and SwiftLint#1889 could be worth looking into, but there are more similar to these.

[AvdLee]

FYI: I'm using the Ruby version of Danger-SwiftLint and I've got the same issue.

[sunshinejr]

#5 is merged and with that and release 0.2.0 of danger-swiftlint, we can now specify directory & configFile, which resolves this issue, e.g:

SwiftLint.lint(directory: "Sources", configFile: ".swiftlint.yml")
SwiftLint.lint(directory: "Tests", configFile: "Tests/HarveyTests/.swiftlint.yml")

More on this setup can be found in Harvey#11

[revolter]

Is it

SwiftLint.lint(directory: "Sources", configFile: ".swiftlint.yml")

swiftlint.lint(directory: "Sources", configFile: ".swiftlint.yml")

swiftlint.lint(directory: "Sources", config_file: ".swiftlint.yml")

swiftlint.lint directory: "Sources" configFile: ".swiftlint.yml"

swiftlint.lint directory: "Sources" config_file: ".swiftlint.yml"

or any of the above? Or lint_files instead of lint?

Note: I'm asking about the syntax that should be used in the Dangerfile.

[sunshinejr]

hey @revolter - this is pretty old and since then the plugin is built into the Danger-Swift itself! if you're looking for a Swift syntax for SwiftLint, here's a doc for ya: https://danger.systems/swift/tutorials/ios_app.html#swiftlint-for-the-greater-good

[revolter]

I know, but I'm using Danger Ruby, not Danger Swift. I don't recall the exact reason for this decision, but I think that there were mentions that Danger Ruby is more mature and has more features.

[revolter]

Now I get it! I was confused because I read the README of the danger-ruby-swiftlint repo, and then it linked me to this issue, which is in the danger-swiftlint repo.

But from the wording of

If you want to learn more details about this, read the whole issue here.
I expected the issue to be in the same repo.

So, I guess that the Ruby version of the SwiftLint Danger plugin doesn't support directory and config_file options, right?

[ashfurrow]

It does support those configuration options; the README is unclear. Basically you need to choose between the following:

• Lint only added or new files, which can use only a single config file because danger-ruby-swiftlint makes an individual swiftlint invocation for each file. Or,
• Lint all files in the repo and get swiftlint to respect the nested config options. This would be slower, though.

Hope I make sense here, still need my coffee ☕ If you have suggestions for updates to the README, I would welcome a PR 🙇

[revolter]

I see. But using the syntax from #4 (comment) you can have the best of both worlds, right? Meaning, you can lint the changed files, while using the correct configuration. 🤔

I actually tried it, and it failed:

[!] Invalid `Dangerfile` file: unknown keywords: directory, config_file. Updating the Danger gem might fix the issue. Your Danger version: 8.2.0, latest Danger version: 8.2.3
#  from Dangerfile:9
#  -------------------------------------------
#  
>  swiftlint.lint_files(directory: 'Sources', config_file: '.swiftlint.yml', fail_on_error: true)
#  swiftlint.lint_files(directory: 'Tests', config_file: 'Tests/.swiftlint.yml', fail_on_error: true)

[ashfurrow]

Hmm, yeah you're right. That should work. I think the problem is that the Swift version and Ruby version of the Danger plugin get their configuration differently. Swift's version passes all the configuration in the invocation (eg: SwiftLint.lint(config_file: ..., directory: ...)) but Ruby's version has a lot of configuration that needs to be set ahead of time, eg:

swiftlint.config_file = '.swiftlint.yml'
swiftlint.directory = "DirectoryA"
swiftlint.lint_files

The properties you set ahead of time are here:

https://github.com/ashfurrow/danger-ruby-swiftlint/blob/d1ac19bbe5a518b7bd5f343aeea84c5df6e2e2c3/lib/danger_plugin.rb#L24-L54

So you might be able to get the code you posted to work with this approach:

swiftlint.config_file = '.swiftlint.yml'
swiftlint.directory = 'Sources'
swiftlint.lint_files(fail_on_error: true)

swiftlint.config_file = 'Tests/.swiftlint.yml'
swiftlint.directory = 'Tests'
swiftlint.lint_files(fail_on_error: true)

I think. Honestly I haven't used this in a while so I'm a little rusty 😅

[revolter]

For some reason, it seems like it concatenates them, and it fails with the error

[!] Invalid Dangerfile file: No such file or directory @ dir_s_chdir - /path/to/repo/Sources/Tests.

Instead of using /path/to/repo/Tests 🤔

[ashfurrow]

Hmm, maybe it's keeping state in between the two invocations. Can you try swapping the order and seeing if you get the same error? Maybe we need like a swiftlint.reset?

[revolter]

[!] Invalid Dangerfile file: No such file or directory @ dir_s_chdir - /path/to/repo/Tests/Sources.

😂

[revolter]

But it should simply overwrite it. Unless there is an explicit piece of code which concatenates the previous value with the new one. 🤔

[revolter]

I tried using

swiftlint.config_file = '.swiftlint.yml'
swiftlint.directory = 'Sources'
swiftlint.lint_files(fail_on_error: true)

swiftlint.config_file = '.swiftlint.yml'
swiftlint.directory = '../Tests'
swiftlint.lint_files(fail_on_error: true)

(and not swiftlint.config_file = 'Tests/.swiftlint.yml' because I found out that config_file is relative to directory)

and I get this output:

Swiftlint will be run from /path/to/repo/Tests
Swiftlint will exclude the following paths: []
Swiftlint includes the following paths: []
linting with options: {:config=>".swiftlint.yml", :reporter=>"json", :quiet=>true, :pwd=>"/path/to/repo/Tests", :force_exclude=>true}
Swiftlint will lint the following files: 
No lintable files found at paths: ''

even though there are files with lint warnings in the Tests directory. And I don't understand why does it say No lintable files found at paths: ''.

[ashfurrow]

Hmm, I'm not really sure. Probably something with File.expand_path.

I don't have a tonne of free time right not to debug this, sorry. I would encourage you to clone the gem and work away at it locally. Most of the logic is in this one file so you should be able to figure it out.

Another option would be to invoke Danger twice, with two configs (ie: point it to two different Dangerfiles). Good luck!

[revolter]

I'm pretty sure it's caused by this line:

https://github.com/ashfurrow/danger-ruby-swiftlint/blob/d1ac19bbe5a518b7bd5f343aeea84c5df6e2e2c3/ext/swiftlint/swiftlint.rb#L12

Changes the current working directory of the process to the given string.

(https://www.rubydoc.info/stdlib/core/Dir.chdir)

Which means that any other plugin called after this might be affected, because the new working directory remains set for the rest of the process.

[revolter]

For others interested, the current workaround was to use:

current_dir = Dir.pwd()

swiftlint.config_file = '.swiftlint.yml'
swiftlint.directory = 'Sources'
swiftlint.lint_files(fail_on_error: true)

Dir.chdir(current_dir)

swiftlint.directory = 'Tests'
swiftlint.lint_files(fail_on_error: true)

Dir.chdir(current_dir)

[ashfurrow]

Good catch! We should do better with scoping that, eg:

Dir.chdir options.delete(:pwd) do
  # pwd is changed inside this block, but is changed back afterwards
end

[revolter]

I created an issue for this, because I spammed this issue enough (sorry 🙈). You could post this proposed solution there 😃

[dogo]

I'm currently using this approach:

# Define a method to handle directory linting
def lint_directory(directory, config_file)
  Dir.chdir(directory) do
    swiftlint.config_file = config_file
    swiftlint.lint_files(fail_on_error: true)
  end
end

# Lint Sources directory
lint_directory('Sources', '.swiftlint.yml')

# Lint Tests directory
lint_directory('Tests', '.swiftlint.yml')