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

Add excluding by prefix option #3345

Merged
merged 4 commits into from
Nov 7, 2020

Conversation

JohnReeze
Copy link
Contributor

As we discussed here #3325 sometimes current excluding algorithm maybe slower than excluding paths by absolute prefix. So I added option for such cases.

Based on what I've checked it works faster for next scenarios:

  • the number of input files is relatively small (e.g. when using use-script-input-files) and excluded directories contain relatively big number of lintable files
  • the number of excluded directories relatively small (e.g. Pods + ThirdParty) and globs not used

But still it depends on many parameters which can be not be automatically evaluated so

@jpsim looking forward to your review =)

@SwiftLintBot
Copy link

SwiftLintBot commented Sep 16, 2020

12 Messages
📖 Linting Aerial with this PR took 1.99s vs 1.94s on master (2% slower)
📖 Linting Alamofire with this PR took 2.77s vs 2.77s on master (0% slower)
📖 Linting Firefox with this PR took 9.33s vs 9.37s on master (0% faster)
📖 Linting Kickstarter with this PR took 15.45s vs 15.5s on master (0% faster)
📖 Linting Moya with this PR took 1.34s vs 1.31s on master (2% slower)
📖 Linting Nimble with this PR took 1.38s vs 1.38s on master (0% slower)
📖 Linting Quick with this PR took 0.63s vs 0.63s on master (0% slower)
📖 Linting Realm with this PR took 4.43s vs 4.46s on master (0% faster)
📖 Linting SourceKitten with this PR took 1.04s vs 1.03s on master (0% slower)
📖 Linting Sourcery with this PR took 7.15s vs 7.18s on master (0% faster)
📖 Linting Swift with this PR took 10.97s vs 10.98s on master (0% faster)
📖 Linting WordPress with this PR took 17.46s vs 17.54s on master (0% faster)

Generated by 🚫 Danger

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2020

Codecov Report

Merging #3345 into master will decrease coverage by 0.03%.
The diff coverage is 69.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3345      +/-   ##
==========================================
- Coverage   90.53%   90.50%   -0.04%     
==========================================
  Files         417      417              
  Lines       20497    20577      +80     
==========================================
+ Hits        18557    18623      +66     
- Misses       1940     1954      +14     
Impacted Files Coverage Δ
Source/swiftlint/Commands/AnalyzeCommand.swift 0.00% <0.00%> (ø)
Source/swiftlint/Commands/AutoCorrectCommand.swift 0.00% <0.00%> (ø)
Source/swiftlint/Commands/LintCommand.swift 0.00% <0.00%> (ø)
...iftlint/Extensions/Configuration+CommandLine.swift 0.00% <0.00%> (ø)
Source/swiftlint/Helpers/CommonOptions.swift 0.00% <ø> (ø)
...ource/swiftlint/Helpers/LintOrAnalyzeCommand.swift 0.00% <0.00%> (ø)
...ource/swiftlint/Helpers/LintableFilesVisitor.swift 0.00% <0.00%> (ø)
...ework/Extensions/Configuration+LintableFiles.swift 97.29% <100.00%> (+0.86%) ⬆️
...s/SwiftLintFrameworkTests/ConfigurationTests.swift 97.30% <100.00%> (+0.77%) ⬆️
...LintFramework/Extensions/SwiftLintFile+Regex.swift 96.74% <0.00%> (-0.47%) ⬇️

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 15c25ab...00fbc7a. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Oct 14, 2020

Codecov Report

Merging #3345 (bf09af9) into master (d628c41) will decrease coverage by 0.02%.
The diff coverage is 69.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3345      +/-   ##
==========================================
- Coverage   90.51%   90.49%   -0.03%     
==========================================
  Files         420      420              
  Lines       20564    20643      +79     
==========================================
+ Hits        18614    18680      +66     
- Misses       1950     1963      +13     
Impacted Files Coverage Δ
Source/swiftlint/Commands/AnalyzeCommand.swift 0.00% <0.00%> (ø)
Source/swiftlint/Commands/AutoCorrectCommand.swift 0.00% <0.00%> (ø)
Source/swiftlint/Commands/LintCommand.swift 0.00% <0.00%> (ø)
...iftlint/Extensions/Configuration+CommandLine.swift 0.00% <0.00%> (ø)
Source/swiftlint/Helpers/CommonOptions.swift 0.00% <ø> (ø)
...ource/swiftlint/Helpers/LintOrAnalyzeCommand.swift 0.00% <0.00%> (ø)
...ource/swiftlint/Helpers/LintableFilesVisitor.swift 0.00% <0.00%> (ø)
...ework/Extensions/Configuration+LintableFiles.swift 97.22% <100.00%> (+0.79%) ⬆️
...s/SwiftLintFrameworkTests/ConfigurationTests.swift 97.30% <100.00%> (+0.77%) ⬆️

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 d628c41...bf09af9. Read the comment docs.

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

@JohnReeze is this working as you intend it to? I wouldn't expect adding this option to impact existing projects, but OSSCheck reported hundreds of changes in this comment.

@jpsim jpsim mentioned this pull request Nov 7, 2020
@JohnReeze
Copy link
Contributor Author

@jpsim It should work (and it does as I use it for about a month). And it's strange that OSSCheck reported 430 warnings because I only added optional parameter which by default is not supposed to impact anything

@JohnReeze
Copy link
Contributor Author

I guess it's somehow connected to #3376 with same 430 warnings)
What am I supposed to do in that case? @jpsim

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

I see you merged master into your branch, let's see if the same thing happens.

@jpsim
Copy link
Collaborator

jpsim commented Nov 7, 2020

Awesome, looks like OSSCheck got confused previously.

Overall, I'm supportive of this change, although I have a small concern of adding diverging behaviors in small ways like this, and the fact that now you'd get different results if you run swiftlint with or without this argument, but I understand that we weren't able to find a way to improve the performance of the current algorithm without changing its behavior.

I'll be pushing some very minor edits to your branch and then will merge when CI is green.

1. Remove likely unnecessary uses of `parallelMap`/`parallelFlatMap`.
   The parallel versions of these methods have a significant amount of
   overhead that doesn't justify their use if the body of the map
   operation is cheap.
2. Remove local variable that is only used to immediately return the
   value.
@jpsim jpsim merged commit fc0092d into realm:master Nov 7, 2020
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.

5 participants