Support for passing multiple paths to swiftlint lint and autocorrect #1191

benasher44 · 2017-01-14T00:27:21Z

This implements #810:

TODO:

What does rootPath mean now (especially in terms of caching) when multiple files are passed? (disabled with a warning)
Okay to get drop the --path option? (decided to keep)
Add tests. (filed CLI arg testing follow up ticket Add tests in CI that test CLI args/options #1213)
Add CHANGELOG entry.

This + pre-commit/pre-commit#467 (personal motivation for this PR) + hooks.yaml (maybe in a follow up PR) makes this work with pre-commit using SwiftPM. I've tested this branch locally with pre-commit, and it works great!

I'd appreciate thoughts and feedback on the above TODOs and existing changes :)

benasher44 · 2017-01-14T00:31:05Z

Source/swiftlint/Extensions/Configuration+CommandLine.swift

                                  quiet: options.quiet, useScriptInputFiles: options.useScriptInputFiles,
                                  cache: cache, parallel: true, visitorBlock: visitorBlock)
    }

    // MARK: AutoCorrect Command

    init(options: AutoCorrectOptions) {
-        self.init(commandLinePath: options.configurationFile, rootPath: options.path, quiet: options.quiet)
+        self.init(commandLinePath: options.configurationFile, rootPath: "", quiet: options.quiet)


This is temporary to get this to compile and test the multiple paths changes.

benasher44 · 2017-01-14T00:31:09Z

Source/swiftlint/Extensions/LinterCache+CommandLine.swift

@@ -45,7 +45,7 @@ private func cacheURL(options: LintOptions, configuration: Configuration) -> URL
 }

 private func defaultCacheURL(options: LintOptions) -> URL {
-    let rootPath = options.path.bridge().absolutePathRepresentation()
+    let rootPath = ""//options.path.bridge().absolutePathRepresentation()


This is temporary to get this to compile and test the multiple paths changes.

SwiftLintBot · 2017-01-14T00:32:29Z

	1 Warning
⚠️	This PR may need tests.

	12 Messages
📖	Linting Aerial with this PR took 0.4s vs 0.38s on master (5% slower)
📖	Linting Alamofire with this PR took 3.53s vs 3.71s on master (4% faster)
📖	Linting Firefox with this PR took 14.21s vs 14.49s on master (1% faster)
📖	Linting Kickstarter with this PR took 21.9s vs 19.75s on master (10% slower)
📖	Linting Moya with this PR took 2.21s vs 1.96s on master (12% slower)
📖	Linting Nimble with this PR took 2.2s vs 1.73s on master (27% slower)
📖	Linting Quick with this PR took 0.56s vs 0.51s on master (9% slower)
📖	Linting Realm with this PR took 3.89s vs 3.29s on master (18% slower)
📖	Linting SourceKitten with this PR took 1.2s vs 1.06s on master (13% slower)
📖	Linting Sourcery with this PR took 5.27s vs 4.64s on master (13% slower)
📖	Linting Swift with this PR took 30.99s vs 28.27s on master (9% slower)
📖	Linting WordPress with this PR took 18.42s vs 16.7s on master (10% slower)

Generated by 🚫 Danger

marcelofabri · 2017-01-14T01:04:00Z

What does rootPath mean now (especially in terms of caching)?

In terms of caching, its hash is used as the default cache file name.

benasher44 · 2017-01-14T16:46:29Z

Yep. Sorry that TODO is poorly worded. I mean that I'm not sure that it has any meaning in this context. There isn't a useful rootPath when you pass multiple files as CLI args, since, even if you calculated the nearest commmon ancestor of all of the paths, you could end up with something like ~ as the rootPath. I think it makes sense to just print that caching is disabled in this instance.

benasher44 · 2017-01-14T16:47:47Z

Does that seem reasonable? Or is there some way I'm missing that caching behavior should be preserved in this instance?

marcelofabri · 2017-01-14T16:48:42Z

I think either disabling or concatenating all files before getting the hash would be ok.

benasher44 · 2017-01-15T00:16:14Z

Okay thanks! I went with concatenating all the files to start. If we're okay dropping --path in favor of this, I'll move forward with tests and changelog.

marcelofabri · 2017-01-15T23:06:50Z

If we're okay dropping --path in favor of this, I'll move forward with tests and changelog.

IMHO it'd be better if we could keep --path too to avoid breaking any existing setups.

marcelofabri · 2017-01-15T23:07:54Z

Source/swiftlint/Extensions/LinterCache+CommandLine.swift

@@ -45,15 +45,15 @@ private func cacheURL(options: LintOptions, configuration: Configuration) -> URL
 }

 private func defaultCacheURL(options: LintOptions) -> URL {
-    let rootPath = options.path.bridge().absolutePathRepresentation()
+    let cacheSignature = options.paths.map { $0.bridge().absolutePathRepresentation() }.joined(separator: "")


"" is the default separator, you should use just .joined() (see #1093)

benasher44 · 2017-01-15T23:18:58Z

Okay! I've been thinking about concatenating the paths as a cache signature, and it feels a bit odd to me. It seems like it'd be better to disable and say up front that a cache path should be provided, which I think would avoid inconsistent/unexpected caching behavior.

benasher44 · 2017-01-15T23:20:02Z

That is, only for the case of multiple paths.

marcelofabri · 2017-01-15T23:28:18Z

Okay! I've been thinking about concatenating the paths as a cache signature, and it feels a bit odd to me. It seems like it'd be better to disable and say up front that a cache path should be provided, which I think would avoid inconsistent/unexpected caching behavior.

Seems good for me!

benasher44 · 2017-01-16T22:05:43Z

Where do I find the tests that test CLI args? In Xcode, I only see the ones that test SwiftLintFramework.

marcelofabri · 2017-01-16T22:07:21Z

Where do I find the tests that test CLI args?

There're no tests for this 🙃

benasher44 · 2017-01-16T22:10:18Z

Ha! Well, there is the note from Danger that says:

Linting Alamofire with this PR took 3.3s vs 3.33s on master (0% faster)

I'm guessing this is referring to running swiftlint? I could add a case that runs it with --path, multiple paths, and both. Right?

benasher44 · 2017-01-16T22:14:13Z

Oh I see it's just there in the Dangerfile. Hm okay. Any recommendations then? I could file a task to include some smoke tests for basic CLI options?

codecov-io · 2017-01-16T23:07:59Z

Codecov Report

Merging #1191 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1191      +/-   ##
==========================================
- Coverage   92.16%   92.02%   -0.15%     
==========================================
  Files         292      292              
  Lines       14738    14763      +25     
==========================================
+ Hits        13584    13585       +1     
- Misses       1154     1178      +24

Impacted Files	Coverage Δ
Source/swiftlint/Helpers/CommonOptions.swift	`0% <0%> (ø)`	⬆️
Source/swiftlint/Commands/LintCommand.swift	`0% <0%> (ø)`	⬆️
Source/swiftlint/Commands/AutoCorrectCommand.swift	`0% <0%> (ø)`	⬆️
...iftlint/Extensions/Configuration+CommandLine.swift	`0% <0%> (ø)`	⬆️
Source/SwiftLintFramework/Models/Command.swift	`98.7% <0%> (+1.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b49ae70...32e8c5b. Read the comment docs.

marcelofabri · 2017-01-19T00:21:06Z

I could file a task to include some smoke tests for basic CLI options?

Good idea!

This seems good to me, but I'd like to get a 👍 from @jpsim on this since it deals with the CLI

benasher44 · 2017-01-19T00:30:34Z

Filed #1213

jpsim · 2017-01-25T00:53:46Z

We need to fix our caching strategy before we do this. We need to encapsulate all the things that can affect the cache.

benasher44 · 2017-01-25T01:00:03Z

Is there an open issue that summarizes the issues and/or possible solutions? I have some thoughts after working on this, but I think it'd be best to chime in there.

marcelofabri · 2017-01-25T01:02:17Z

There are #1240 and #1184, but I'm afraid there may be more problems that aren't mentioned on them.

benasher44 · 2017-01-25T01:04:13Z

@marcelofabri or @jpsim could either of you open a mega thread/issue?

jpsim · 2017-01-25T01:06:19Z

#1184 should be that umbrella issue

benasher44 · 2017-02-03T16:13:20Z

Updated to only do automatic caching for single directories. Does that make this a breaking change?

jpsim · 2017-02-15T03:29:16Z

Hi @benasher44, sorry this has been on hiatus for a while. I'd like to see us fully address our caching model (#1184) before we make more changes that could affect it.

benasher44 · 2017-02-15T18:55:27Z

@jpsim no problem! I'd appreciate another ping here once it's resolved, and then I'll rebase and get this PR back in order. Thanks!

marcelofabri · 2017-05-22T06:20:08Z

This could be revisited now that #1530 was merged

benasher44 · 2017-05-22T12:37:36Z

Awesome! Will try to revive it this week! Thanks!

benasher44 · 2017-05-27T19:22:53Z

@marcelofabri I've updated with master, and I think this is getting close to being ready to go. With caching issues being resolved, this is definitely more straightforward.

The next thing to figure out is Configuration.rootPath. It doesn't appear to be used anywhere, since it was previously more important for caching. The only place I see it used it to compare Configuration instances. In the new world of multiple paths, rootPath doesn't really make sense (as it stands now), unless we think it's important to calculate the nearest common ancestor path of all paths passed to swiftlint. Thoughts? Maybe I should remove rootPath?

shepting · 2018-01-16T21:46:59Z

This will be awesome. Looking forward to seeing it merge.

esteluk · 2018-05-14T11:32:22Z

Is there any appetite for reviving this one? Happy to resolve the conflicts if it's no longer on @benasher44's radar.

benasher44 · 2018-05-14T19:28:50Z

I'll take a crack at updating this PR this week

benasher44 · 2018-05-18T22:37:05Z

Okay this is working again. I don't feel good about having no tests. Any advice on the testing front?

esteluk · 2018-05-22T12:55:33Z

Works great for me!

The output for swiftlint help lint is a bit confusing though - is there a way to tidy it up?

[[""]]
	list of paths to the files or directories to lint

esteluk · 2018-05-22T13:52:25Z

I think there might also be a problem with using --path now?
My script runs swiftlint --path foo/bar.swift --config foo/.swiftlint.yml which throws:
Missing argument for --config

benasher44 · 2018-05-25T17:59:30Z

@esteluk thanks for that great feedback! I believe I've fixed the bug you found, so it'd be great if you could give this latest update another go.

For the help message issue, I've filed an issue on Commandant: Carthage/Commandant/issues/128

esteluk · 2018-06-14T11:01:38Z

Sorry, had a play and it seems to work for my purposes at least! Not sure if the maintainers can take another look? :)

jpsim · 2018-06-23T22:05:04Z

I think this is a good time to merge this @benasher44. Sorry for the massive inexcusable delay. Mind rebasing first to address the merge conflict? Thanks!

jpsim · 2018-06-23T22:02:53Z

Source/swiftlint/Commands/AutoCorrectCommand.swift

+            } else {
+                allPaths = paths
+            }
+            return self.init(paths: Array(allPaths), configurationFile: configurationFile, useScriptInputFiles: useScriptInputFiles, quiet: quiet, forceExclude: forceExclude, format: format, cachePath: cachePath, ignoreCache: ignoreCache, useTabs: useTabs)


Shouldn't need to copy allPaths into a new array here. s/Array(allPaths)/allPaths/

jpsim · 2018-06-23T22:03:03Z

Source/swiftlint/Commands/LintCommand.swift

+            } else {
+                allPaths = paths
+            }
+            return self.init(paths: Array(allPaths), useSTDIN: useSTDIN, configurationFile: configurationFile, strict: strict, lenient: lenient, forceExclude: forceExclude, useScriptInputFiles: useScriptInputFiles, benchmark: benchmark, reporter: reporter, quiet: quiet, cachePath: cachePath, ignoreCache: ignoreCache, enableAllRules: enableAllRules)


Shouldn't need to copy allPaths into a new array here. s/Array(allPaths)/allPaths/

marcelofabri · 2018-07-23T19:41:52Z

Thanks, @benasher44 🎉

benasher44 · 2018-07-23T22:05:13Z

🎉 🎉 Thanks for your help @marcelofabri and @jpsim!!

benasher44 commented Jan 14, 2017

View reviewed changes

marcelofabri reviewed Jan 15, 2017

View reviewed changes

benasher44 force-pushed the basher_multiple_paths branch from cf02c33 to 2f61ea8 Compare January 16, 2017 22:00

benasher44 changed the title ~~[WIP] Support for passing multiple paths to swiftlint lint and autocorrect~~ Support for passing multiple paths to swiftlint lint and autocorrect Jan 17, 2017

marcelofabri requested a review from jpsim January 19, 2017 00:21

benasher44 mentioned this pull request Jan 19, 2017

Add tests in CI that test CLI args/options #1213

Open

benasher44 force-pushed the basher_multiple_paths branch from d4009ef to c9098ac Compare January 19, 2017 00:31

benasher44 force-pushed the basher_multiple_paths branch from 537295e to 7452799 Compare May 27, 2017 19:16

benasher44 force-pushed the basher_multiple_paths branch from 1e43731 to 454eff1 Compare May 27, 2017 19:39

benasher44 force-pushed the basher_multiple_paths branch from 454eff1 to 09b7956 Compare May 18, 2018 21:54

benasher44 force-pushed the basher_multiple_paths branch from f197981 to fa9f6e6 Compare May 18, 2018 22:46

benasher44 mentioned this pull request May 25, 2018

Help message for Argument could be improved Carthage/Commandant#128

Closed

jpsim approved these changes Jun 23, 2018

View reviewed changes

benasher44 added 4 commits July 23, 2018 10:13

Support for multiple paths

7a32b8d

Prefer single path, if it's provided; fix multi-path

3f1acd6

Fix paths arg eating other params

7f86dc4

Fix extra array copies

32e8c5b

benasher44 force-pushed the basher_multiple_paths branch from eedbcd8 to 32e8c5b Compare July 23, 2018 17:14

marcelofabri merged commit c7c0ac8 into realm:master Jul 23, 2018

benasher44 deleted the basher_multiple_paths branch July 23, 2018 22:05

Support for passing multiple paths to swiftlint lint and autocorrect #1191

Support for passing multiple paths to swiftlint lint and autocorrect #1191

Conversation

benasher44 commented Jan 14, 2017 • edited Loading

benasher44 Jan 14, 2017

Choose a reason for hiding this comment

benasher44 Jan 14, 2017

Choose a reason for hiding this comment

SwiftLintBot commented Jan 14, 2017 • edited Loading

marcelofabri commented Jan 14, 2017

benasher44 commented Jan 14, 2017 • edited Loading

benasher44 commented Jan 14, 2017

marcelofabri commented Jan 14, 2017

benasher44 commented Jan 15, 2017

marcelofabri commented Jan 15, 2017

marcelofabri Jan 15, 2017

Choose a reason for hiding this comment

benasher44 commented Jan 15, 2017

benasher44 commented Jan 15, 2017

marcelofabri commented Jan 15, 2017

benasher44 commented Jan 16, 2017

marcelofabri commented Jan 16, 2017

benasher44 commented Jan 16, 2017 • edited Loading

benasher44 commented Jan 16, 2017

codecov-io commented Jan 16, 2017 • edited Loading

Codecov Report

marcelofabri commented Jan 19, 2017

benasher44 commented Jan 19, 2017

jpsim commented Jan 25, 2017

benasher44 commented Jan 25, 2017

marcelofabri commented Jan 25, 2017

benasher44 commented Jan 25, 2017

jpsim commented Jan 25, 2017

benasher44 commented Feb 3, 2017

jpsim commented Feb 15, 2017

benasher44 commented Feb 15, 2017

marcelofabri commented May 22, 2017

benasher44 commented May 22, 2017

benasher44 commented May 27, 2017

shepting commented Jan 16, 2018

esteluk commented May 14, 2018

benasher44 commented May 14, 2018

benasher44 commented May 18, 2018

esteluk commented May 22, 2018

esteluk commented May 22, 2018

benasher44 commented May 25, 2018

esteluk commented Jun 14, 2018

jpsim commented Jun 23, 2018

jpsim Jun 23, 2018

Choose a reason for hiding this comment

jpsim Jun 23, 2018

Choose a reason for hiding this comment

marcelofabri commented Jul 23, 2018

benasher44 commented Jul 23, 2018

benasher44 commented Jan 14, 2017 •

edited

Loading

SwiftLintBot commented Jan 14, 2017 •

edited

Loading

benasher44 commented Jan 14, 2017 •

edited

Loading

benasher44 commented Jan 16, 2017 •

edited

Loading

codecov-io commented Jan 16, 2017 •

edited

Loading