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

Projects
None yet
3 participants
@yhkaplan
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Jul 22, 2018

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

This comment has been minimized.

@SD10

SD10 Jul 22, 2018

Member

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

This comment has been minimized.

@yhkaplan

yhkaplan Jul 22, 2018

Contributor

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?

This comment has been minimized.

@SD10

SD10 Jul 22, 2018

Member

Yep, I think that's a good idea 👍

This comment has been minimized.

@yhkaplan

yhkaplan Jul 22, 2018

Contributor

Added here: 91d498e
Tests added here: f85c834

@orta

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

orta commented Jul 22, 2018

Perfect 👍

@orta orta merged commit 03c9bb3 into danger:master Jul 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@peril-staging

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Jul 22, 2018

@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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

yhkaplan commented Jul 22, 2018

Fair enough. 👍

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