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

Support non Swift files in --file argument #1755

Merged
merged 1 commit into from
Aug 8, 2017

Conversation

marcelofabri
Copy link
Collaborator

Fixes #1721.

@SwiftLintBot
Copy link

SwiftLintBot commented Aug 7, 2017

12 Messages
📖 Linting Aerial with this PR took 0.36s vs 0.34s on master (5% slower)
📖 Linting Alamofire with this PR took 2.51s vs 2.44s on master (2% slower)
📖 Linting Firefox with this PR took 10.2s vs 10.6s on master (3% faster)
📖 Linting Kickstarter with this PR took 15.27s vs 15.56s on master (1% faster)
📖 Linting Moya with this PR took 1.06s vs 1.06s on master (0% slower)
📖 Linting Nimble with this PR took 1.42s vs 1.4s on master (1% slower)
📖 Linting Quick with this PR took 0.45s vs 0.46s on master (2% faster)
📖 Linting Realm with this PR took 2.23s vs 2.2s on master (1% slower)
📖 Linting SourceKitten with this PR took 0.8s vs 0.82s on master (2% faster)
📖 Linting Sourcery with this PR took 3.7s vs 3.64s on master (1% slower)
📖 Linting Swift with this PR took 10.53s vs 10.46s on master (0% slower)
📖 Linting WordPress with this PR took 9.41s vs 9.84s on master (4% faster)

Generated by 🚫 Danger

@marcelofabri marcelofabri force-pushed the mf-lint-non-swift-file branch from 542f91e to bc00097 Compare August 7, 2017 12:41
@codecov-io
Copy link

codecov-io commented Aug 7, 2017

Codecov Report

Merging #1755 into master will increase coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1755      +/-   ##
=========================================
+ Coverage   87.78%   87.8%   +0.02%     
=========================================
  Files         219     219              
  Lines       10730   10730              
=========================================
+ Hits         9419    9422       +3     
+ Misses       1311    1308       -3
Impacted Files Coverage Δ
...ework/Extensions/Configuration+LintableFiles.swift 84.61% <50%> (ø) ⬆️
...iftLintFramework/Extensions/String+SwiftLint.swift 68.51% <0%> (+5.55%) ⬆️

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 8c98a4f...a300409. Read the comment docs.

@@ -21,7 +21,7 @@ extension FileManager: LintableFileManager {
.standardizingPath

// if path is a file, it won't be returned in `enumerator(atPath:)`
if absolutePath.bridge().isSwiftFile() && absolutePath.isFile {
if absolutePath.isFile {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change? I think the lintablePaths change is sufficient to support the lint --path file.notswift scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's true, but when exactly would this check be true then? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when specifying included and excluded configuration values, which IMO doesn't really make sense to specify non-Swift files in there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda worried that if we extend included/excluded to support globbing, this change would attempt to lint non-Swift files if Sources/** were specified, for example.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it 👍

@marcelofabri marcelofabri force-pushed the mf-lint-non-swift-file branch from bc00097 to a300409 Compare August 7, 2017 21:10
@marcelofabri marcelofabri merged commit cb77820 into master Aug 8, 2017
@marcelofabri marcelofabri deleted the mf-lint-non-swift-file branch August 8, 2017 06:25
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.

4 participants