-
Notifications
You must be signed in to change notification settings - Fork 16
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 directory & configFile options to SwiftLint #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great change in general :) Helpful for external libraries for sure.
Left two nits and one opinion which I'm not really sure what to do about 🤷♂️
@@ -10,7 +10,7 @@ class DangerSwiftLintTests: XCTestCase { | |||
override func setUp() { | |||
executor = FakeShellExecutor() | |||
// This is for me, testing. Uncomment if you're running tests locally. | |||
FileManager.default.changeCurrentDirectoryPath("/Users/ash/bin/danger-swiftlint") | |||
FileManager.default.changeCurrentDirectoryPath("/Users/lukaszmroz/Projects/OtherProjects/Libraries/danger-swiftlint") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to revert this guy to the original ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awww, I was pretty sure I didn't pushed this one, nice catch!
@@ -21,6 +21,30 @@ class DangerSwiftLintTests: XCTestCase { | |||
XCTAssertNotEqual(executor.invocations.dropFirst().count, 0) | |||
} | |||
|
|||
func testExecutesSwiftLintWithConfigWhenPassed() { | |||
let configFile = "/Path/to/config/.swiftlint.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a continuous test - shouldn't we just bundle a .swiftlint.yml
mock in the repo ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, this is just needed to check if the argument with this path was added.
@@ -8,10 +8,10 @@ public struct SwiftLint { | |||
/// This is the main entry point for linting Swift in PRs using Danger-Swift. | |||
/// Call this function anywhere from within your Dangerfile.swift. | |||
@discardableResult | |||
public static func lint() -> [Violation] { | |||
public static func lint(directory: String? = nil, configFile: String? = nil) -> [Violation] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the configFile
name is so pretty, I don't super-like it but I don't have a nicer idea at the moment 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all ears for a better name 😄 didn't think of a better one as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if a good alternative exists. It's the parameter used to specify the file where the config is 🙃
P.S no biggie @ashfurrow , but I think you forgot to turn on "Build forked pull requests", since CircleCI didn't actually do its thing here :) |
Good catch, I have re-delivered the webhook event for the last commit in this PR to re-trigger the build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to merge, thanks! Just the CI failure at this point.
Okay, fixed all! First there was a problem with initializing It should be ready to go @ashfurrow |
Thanks a lot for contributing @sunshinejr! You've been invited to be a collaborator on this repo – no pressure to accept! If you'd like moreinformation on what this means, check out the Moya contributor guidelines and feel free to reach out with any questions. Generated by 🚫 dangerJS |
Cooooooool, tagged a 0.2.0 release with these changes. Thanks again!! |
Hey Ash!
So this is a workaround for #4. This shouldn't break any current configurations, as I've added default arguments. You can see a working copy of this one at Harvey#11.