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 an option to filter files and to disable quiet mode. #308

Merged
merged 12 commits into from
Jan 9, 2020

Conversation

AvdLee
Copy link
Contributor

@AvdLee AvdLee commented Jan 8, 2020

This PR adds two new features. The first one enables a files filter that can be used as follows:

SwiftLint.lint(inline: true, configFile: "\(pwd)/Submodules/WeTransfer-iOS-CI/SwiftLint/.swiftlint-tests.yml", quiet: true, filesFilter: { file -> Bool in
    return file.lowercased().contains("test")
})

It allows us to use a specific config file for certain files only. I've decided to implement it this way as the excluded paths are not respected by Danger-SwiftLint. The Ruby version we've been using before does something similar as you can see here. We needed the same as we migrate to Danger-Swift.

To debug this new feature I decided to also add an option to disable the --quiet argument. This can be useful for debugging purposes to see whether the used configurations are respected:

Loading configuration from '/Users/antoinevanderlee/Documents/GIT-Projects/WeTransfer/Coyote/Submodules/WeTransfer-iOS-CI/SwiftLint/.swiftlint-source.yml'

Linting 'ContentUploadInfoList.swift' (1/2)
Linting 'ChunksCreationOperation.swift' (2/2)

Done linting! Found 1 violation, 1 serious in 2 files.

Loading configuration from '/Users/antoinevanderlee/Documents/GIT-Projects/WeTransfer/Coyote/Submodules/WeTransfer-iOS-CI/SwiftLint/.swiftlint-tests.yml'

Linting 'ChunksCreationOperationTests.swift' (1/1)

Done linting! Found 0 violations, 0 serious in 1 file.

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @AvdLee, this is a really useful change!
I would suggest to take a different approach, instead of changing the current function, we could expose a new one

    public static func lint(
                            files: [File],
                            inline: Bool = false,
                            configFile: String? = nil, 
                            strict: Bool = false, 
                            configFile: String? = nil,
                            quiet: Bool = true,
                            swiftlintPath: String? = nil) -> [SwiftLintViolation] {	                            swiftlintPath: String? = nil) -> [SwiftLintViolation] {

This could potentially be called by the other function when lint all files is false, then we can reuse some logic instead of duplicate it.
Those are the benefits I see with this approach:

  • We would be removing an impossible state, then
        if filesFilter != nil && lintAllFiles {
            failAction("You can't use a files filter when lintAllFiles is set to `true`.")
            return []
        }

would not be a problem anymore, because would not be a possible case

  • The possibility of passing the files yourself could be helpful in some corner cases, for example when you have a file that is automatically generated (from Sourcery for example), then is on .gitignore, but you still want to have your generated file respecting your linting rule, and you want colleagues to change the stencil file in order to make the generated file pass the swiftlint, you could pass the generated file path to the linter manually.

There is another possibility that could be even better, but requires some additional work.
We could deprecate the old function and create one that takes an enum:

enum LintStyle {
  case all(directory: String?)
  case modifiedFiles(directory: String?)
  case files([File])
}

And then have

	    public static func lint(
                            lintStyle: LintStyle = .modfiedFiles(directory: nil)
                            inline: Bool = false,                         
                            configFile: String? = nil,
                            strict: Bool = false,
                            quiet: Bool = true,
                            swiftlintPath: String? = nil) -> [SwiftLintViolation] {	                          

This function would have a switch and based on the lintStyle would decide what to do, and could re use the logic of the current function.
This would allow us to just call this new function from the current one, something like:

    public static func lint(inline: Bool = false,
                            configFile: String? = nil,
                            strict: Bool = false,
                            quiet: Bool = true,
                            lintAllFiles: Bool = false,
                            swiftlintPath: String? = nil) -> [SwiftLintViolation] {
    lint(lintAll ? .all : .modifiedFiles,
           configFile: configFile,
           strict: strict,
           quiet: quiet,
           lintAllFiles: lintAllFiles,
           swiftlintPath: swiftlintPath)
 }

Given this change is going to go on the 3.0.0 we could even make a breaking change, but I think deprecating the old one would be better.

What do you think?

@AvdLee
Copy link
Contributor Author

AvdLee commented Jan 9, 2020

Thanks for your detailed reply, @f-meloni! You're almost perfectly describing my path of thinking while developing this implementation. I thought about both your options but decided to keep it simple for now.

The possibility of passing the files yourself could be helpful in some corner cases, for example when you have a file that is automatically generated (from Sourcery for example), then is on .gitignore, but you still want to have your generated file respecting your linting rule, and you want colleagues to change the stencil file in order to make the generated file pass the swiftlint, you could pass the generated file path to the linter manually.

I didn't think about this. I thought it would be a downside to not rely on the basic filtering and File gathering we already have in place with the added + modifiedFiles filtered by .swift extension.
However, thinking about this scenario, it's indeed useful to be able to pass in Files. It allows us to do both filtering ánd adding additional files.

We could deprecate the old function and create one that takes an enum

This is indeed the first thing you think of once you see more than two similar kinds of methods appear. Once again, I decided to keep it simple but from the beginning, this would have been my preference. As you suggest it here now as well, I'll rewrite my code to make use of the enum and deprecate the other methods starting from version 3.0.0

You can expect the changes coming in later today.

@AvdLee
Copy link
Contributor Author

AvdLee commented Jan 9, 2020

Calling SwiftLint now looks as followed for us:

let pwd = danger.utils.exec("pwd")
        print("Starting SwiftLint...")

        let files = danger.git.createdFiles + danger.git.modifiedFiles
        print("Linting filesss:\n- \(files.joined(separator: "\n- "))")

        let splittedFiles = files.split(whereSeparator: { $0.lowercased().contains("test") })
        let nonTestFiles = Array(splittedFiles.first ?? [])
        let testFiles = Array(splittedFiles.last ?? [])

        SwiftLint.lint(.files(nonTestFiles),
                       inline: true,
                       configFile: "\(pwd)/Submodules/WeTransfer-iOS-CI/SwiftLint/.swiftlint-source.yml",
                       quiet: false)

        SwiftLint.lint(.files(testFiles),
                       inline: true,
                       configFile: "\(pwd)/Submodules/WeTransfer-iOS-CI/SwiftLint/.swiftlint-tests.yml",
                       quiet: false)

I'll work on fixing the Danger warnings next.

Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

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

Really cool, thank you!

@f-meloni f-meloni merged commit 0bdb5ba into danger:master Jan 9, 2020
@peril-staging
Copy link
Contributor

peril-staging bot commented Jan 12, 2020

Thanks for the PR @AvdLee.

This PR has been shipped in v3.0.0 - CHANGELOG.

@AvdLee AvdLee deleted the feature/files-filter branch January 13, 2020 08:51
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.

2 participants