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

Return false for isFile for empty strings #2706

Merged
merged 2 commits into from
Apr 6, 2019
Merged

Return false for isFile for empty strings #2706

merged 2 commits into from
Apr 6, 2019

Conversation

keith
Copy link
Collaborator

@keith keith commented Apr 3, 2019

@marcelofabri
Copy link
Collaborator

Why are we passing "" in the first place? 🤔

@SwiftLintBot
Copy link

SwiftLintBot commented Apr 3, 2019

12 Messages
📖 Linting Aerial with this PR took 2.05s vs 1.89s on master (8% slower)
📖 Linting Alamofire with this PR took 4.83s vs 4.35s on master (11% slower)
📖 Linting Firefox with this PR took 14.64s vs 13.08s on master (11% slower)
📖 Linting Kickstarter with this PR took 22.77s vs 22.26s on master (2% slower)
📖 Linting Moya with this PR took 2.02s vs 1.99s on master (1% slower)
📖 Linting Nimble with this PR took 1.85s vs 1.84s on master (0% slower)
📖 Linting Quick with this PR took 0.58s vs 0.58s on master (0% slower)
📖 Linting Realm with this PR took 3.71s vs 3.59s on master (3% slower)
📖 Linting SourceKitten with this PR took 1.2s vs 1.19s on master (0% slower)
📖 Linting Sourcery with this PR took 4.39s vs 4.34s on master (1% slower)
📖 Linting Swift with this PR took 27.87s vs 27.29s on master (2% slower)
📖 Linting WordPress with this PR took 24.04s vs 23.73s on master (1% slower)

Generated by 🚫 Danger

@keith
Copy link
Collaborator Author

keith commented Apr 3, 2019

It looks like that's the default if no path is passed

func pathOption(action: String) -> Option<String> {
return Option(key: "path",
defaultValue: "",
usage: "the path to the file or directory to \(action)")
}

@keith
Copy link
Collaborator Author

keith commented Apr 3, 2019

Maybe that default should be a . though?

@marcelofabri
Copy link
Collaborator

Maybe that default should be a . though?

We can investigate and change, but I'm a bit afraid because we don't have unit tests for the swiftlint target. IMO we can merge this as is and then follow up.

@jpsim
Copy link
Collaborator

jpsim commented Apr 6, 2019

Fix looks good to me. Added a changelog entry in 9cc97d6. Will merge when CI passes.

@jpsim jpsim merged commit ace6aad into master Apr 6, 2019
@jpsim jpsim deleted the ks/linux-fix branch April 6, 2019 22:53
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