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 filetype extensions to modifiedFiles, createdFiles, and deletedFiles #81

Merged
merged 14 commits into from
Jul 22, 2018

Conversation

yhkaplan
Copy link
Contributor

@yhkaplan yhkaplan commented Jul 22, 2018

As per #80, I propose to change modifiedFiles, createdFiles, and deletedFiles to be of type File from String, adding Name and FileType properties.

This will break some Dangerfile workflow, requiring users to potentially have to access the .name property to get a regular string. In return, users a get type-safe enum property called fileType for .h, .json, .m, .markdown, .mm, .pbxproj, .plist, .storyboard, .swift, .xcscheme, .yaml, .yml.

In Ruby, detecting the filetype is rather straightforward because of easy-to-use regex support built right in. Swift, on the other hand, would require users to use the clunky NSRegularExpression class, which is bothersome to do inside the Dangerfile.

Example of use in Dangerfile

import Danger

let codeWasModified = git.modifiedFiles
    .filter { $0.fileType == .swift || $0.fileType == .m  || $0.fileType == .h }
    .count > 0

let changelogWasModified == git.modifiedFiles
    .filter { $0.name == "Changelog.md" }
    .count > 0

if codeWasModified && !changelogWasModified {
    warn("Please update the changelog")
}

@yhkaplan yhkaplan changed the title Feature/add filetype extensions Add filetype extensions to modifiedFiles, createdFiles, and deletedFiles Jul 22, 2018
@yhkaplan
Copy link
Contributor Author

I must have bad luck: Error: Error: Results passed to Danger JS did not include fails.

// MARK: - FileType

public enum FileType: String, Equatable {
case h, json, m, markdown = "md", pbxproj, plist, storyboard, swift
Copy link
Member

@SD10 SD10 Jul 22, 2018

Choose a reason for hiding this comment

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

What do you think about adding the extensions for .xcscheme, .yml and .mm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great! One concern is .yml versus .yaml and how to support it in a simple and easy-to-maintain way. Maybe separating them into two different cases? What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think that's a good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: 91d498e
Tests added here: f85c834

@orta
Copy link
Member

orta commented Jul 22, 2018

Error: Error: Results passed to Danger JS did not include fails I feel is a weird thing we've been seeing in danger-js, I don't think it's your issue

@orta
Copy link
Member

orta commented Jul 22, 2018

Perfect 👍

@orta orta merged commit 03c9bb3 into danger:master Jul 22, 2018
@peril-staging
Copy link
Contributor

peril-staging bot commented Jul 22, 2018

Thanks for the PR @yhkaplan.

This PR has been shipped in v0.4.1 - CHANGELOG.

@yhkaplan
Copy link
Contributor Author

@orta @SD10 Thanks for the quick review!

One concern I’ve had lately is that Danger-js errors can make running Danger-Swift more of a maintenance burden than say the original Ruby Danger. Do you think this is something that should be solved by perfecting Danger-js, or perhaps by having Danger-swift eventually run independently of Danger-js?

@orta
Copy link
Member

orta commented Jul 22, 2018

Danger JS is less mature, that's all. That bug has been real hard to get in danger-js.

FWIW, I'll never build danger for Swift myself, too small of a community (and largely inactive in contributing back) for such a large amount of my time, this technique allows danger-swift to actually exist and to potentially allow it to run on Peril.

@yhkaplan
Copy link
Contributor Author

Fair enough. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants