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

cannot lint multiple files when using --path command option #3923

Conversation

coffmark
Copy link
Contributor

Hello, i'm @coffmark. thank you for all your maintenance & update.

motivation

  1. swiftlint lint --fix --format --path {first-file-path} {second-file-path}
    This command only executes {first-file-path}, i expect execute {first-file-path} and {second-file-path}

  2. swiftlint lint --fix --format {first-file-path} {second-file-path}
    This command executes {first-file-path} & {second-file-path}, this is as expected.

It only doesn't work if --path command option is present.

solution

when execute swiftlint lint --fix --format --path {first-file-path} {second-file-path}

@Option(help: ...)
var path: String? // { is assigned to first-file-path }

@Argument(help: ...)
var paths = [String]() // { is assigned to second-file-path }

Now, allpaths only gets path, so paths are ignored.
The solution is to add paths and path to allpaths**

Looking forward to hearing back from you!

@coffmark coffmark marked this pull request as ready for review March 29, 2022 02:30
@SimplyDanny
Copy link
Collaborator

Looks basically good to me. However, I wonder what the point of the --path option is in the first place.

@coffmark
Copy link
Contributor Author

@SimplyDanny , Thanks for getting back to me!

[--path (string)]
	the path to the file or directory to lint

i'm sorry that i was wrong.
When using path option, can specify only 1 file or 1 directory.

If i specify more than one file, i'll avoid using the path option.

@coffmark coffmark closed this Mar 30, 2022
@SimplyDanny
Copy link
Collaborator

My intention was not to decline your PR. Actually, I think it is valid and should be merged.

My comment above was rather a question about the benefit of the --path option in general. My feeling is that it is redundant and we can get rid of it completely. Could be I miss a use case which requires it, though. Do I, @jpsim, or could/should we at least deprecate it in a follow-up commit?

However, this is a separate discussion and should not influence your change. It just brought this question up. Sorry for the confusion, @coffmark!

@SimplyDanny SimplyDanny reopened this Mar 31, 2022
@SimplyDanny SimplyDanny self-requested a review April 1, 2022 06:44
@coffmark
Copy link
Contributor Author

coffmark commented Apr 3, 2022

Your message encouraged me, Thanks!
I have my doubts about the benefits of the --path option, too.
Thank you, @SimplyDanny!

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 4, 2022

12 Messages
📖 Linting Aerial with this PR took 0.98s vs 0.98s on master (0% slower)
📖 Linting Alamofire with this PR took 1.1s vs 1.11s on master (0% faster)
📖 Linting Firefox with this PR took 4.11s vs 4.2s on master (2% faster)
📖 Linting Kickstarter with this PR took 7.36s vs 7.47s on master (1% faster)
📖 Linting Moya with this PR took 4.41s vs 4.41s on master (0% slower)
📖 Linting Nimble with this PR took 0.4s vs 0.4s on master (0% slower)
📖 Linting Quick with this PR took 0.17s vs 0.17s on master (0% slower)
📖 Linting Realm with this PR took 9.92s vs 10.22s on master (2% faster)
📖 Linting SourceKitten with this PR took 0.33s vs 0.33s on master (0% slower)
📖 Linting Sourcery with this PR took 2.02s vs 2.02s on master (0% slower)
📖 Linting Swift with this PR took 3.18s vs 3.15s on master (0% slower)
📖 Linting WordPress with this PR took 7.53s vs 7.37s on master (2% slower)

Generated by 🚫 Danger

@SimplyDanny
Copy link
Collaborator

Would you please rebase to fix the conflicts, @coffmark? I guess, this would also fix the failed test.

@SimplyDanny
Copy link
Collaborator

There went something wrong with the rebase. There are now too many changed files reverting some previously made changes. I suggest to rebase onto the current master branch interactively removing all the commits on your branch except for the one you created yourself.

@coffmark coffmark force-pushed the feature/lint-multiple-files-using-path-option branch 3 times, most recently from 05d6fc4 to 62ba153 Compare April 11, 2022 23:22
@coffmark
Copy link
Contributor Author

Special thanks for your advice, @SimplyDanny.

Test code in azure pipeline seems to be failing due to increased Swift version. (Link)

Could you please confirm this, #3944 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants