Automate PR checking with Danger

[rnystrom]

http://danger.systems/ cc @orta

Warnings

• [ ] Non-final class in examples
• [x] Swiftlint for examples
• [ ] Markdown verification using prose (incl. link checking)
• [ ] ObjC classes without IG_SUBCLASSING_RESTRICTED
• [x] ~Big PR~
• [x] ~No CHANGELOG.md entry for any change~

Failures

• [ ] New CHANGELOG.md entry when .h file is changed
• [ ] Facebook copyright in headers
• [ ] Changes to restricted file types (.lock, .xcconfig, etc)
• [ ] Always fail: "Merging should be done internally"
• [x] ~Only allow editing /docs from admins~
• [x] ~WIP pull requests~

Any other ideas?

[Sherlouk]

Failure on changes to Pod files (.lock, .xcconfig) Source https://github.com/Instagram/IGListKit/pull/368#issuecomment-269092317

In the past I know Jesse has done some WIP pull requests, maybe a warning/failure if the PR title has [WIP] in it?

Also have you considered getting some sort of code linting in place? Would be good to have that in place to capture some nits/good structure (and add a failure for that)

Could we maybe get rid of my super janky markdown link checker from Travis and use prose instead? (Will cover things like spelling, links, etc in all our guides/markdown files)

[jessesquires]

Danger plugins:

• Proselint: https://github.com/dbgrandi/danger-prose
• Swiftlint: https://github.com/ashfurrow/danger-swiftlint

[Sherlouk]

Could probably also do with a changelog format validator. {summary} {Name} (#{PR Number})

Should be easy to at least validate the correct PR and PR link

[orta]

There's a semi-standard for CHANGELOGs too - this plugin implements it - http://danger.systems/plugins/changelog.html

looks like https://github.com/ruby-grape/grape/blob/master/CHANGELOG.md

[Sherlouk]

As discussed in #399, we should always fail with a message similar to "Merging should be done internally" to prevent the merge button from ever being usable.

@rnystrom Should I go ahead and update the original comment with the various rules (as discussed above in thread) so we can tick them off once done?

[rnystrom]

@Sherlouk go for it!

Some linter ideas:

• Error on Swift classes in the examples not marked final
• Warn on ObjC classes w/out using IG_SUBCLASSING_RESTRICTED

[Sherlouk]

@rnystrom Assume you meant classes not marked final for Swift?

[Sherlouk]

@rnystrom Worth keeping this issue open as there are still unimplemented rules?

[rnystrom]

@Sherlouk good call!

[jessesquires]

updated original issue to use markdown checklists 😄

[Iron-Ham]

@jessesquires taking this on re: SwiftLint. I'm assuming y'all also want the script running in the example build phases? I have a swiftlint.yml that I pop into all of my projects -- should i start there, or with the default ruleset: https://gist.github.com/heshamsalman/0910500186d4372f0c0d52ac73ea2110

[jessesquires]

thanks @heshamsalman !

let's start with the default rules 😄

[Iron-Ham]

Heads up, there's gonna be a clean-up task immediately after this for the examples. Staring down the barrel of 228 warnings with default rules.

[jessesquires]

@heshamsalman - No worries. Let's do it in 1 PR with individual commits for each step so we can review it easier. 1 commit for swiftLint, 1 commit to fixup each example

[rnystrom]

@jessesquires @heshamsalman is there an "ObjC lint"? This is so cool.

[Iron-Ham]

@rnystrom there are a few different objective c linters around -- I know of OCLint but haven't used it.

I'll look into it and report back

[orta]

Not strictly a linter, but we use Space commander by square for the syntax parts of it

[Sherlouk]

Do we know if there's a way to have Travis and Danger as separate integrations on GitHub?

Would be nice to know if a build failed because of tests/etc vs Danger without having to go through the build log!

[orta]

Get Danger to print your test results?

[orta]

Or use Circle CI for Danger

[Iron-Ham]

OCLint seems pretty similar to Swiftlint. Not quite as user friendly, but great overall.

[Sherlouk]

So it turns out prose is a lot more annoying to do than I was first hoping...

Not only do you danger-prose, but you also need proselint and mdspell. To get proselint you also need pip.

Lots of dependencies which we know Travis doesn't really like (and would add quite a lot of time to builds until caching is fully implemented #757)

[Iron-Ham]

Something tells me pip dependencies lead to a six error on travis.

On May 24, 2017, 5:42 PM -0400, James Sherlock notifications@github.com, wrote:

So it turns out prose is a lot more annoying to do than I was first hoping...

Not only do you danger-prose, but you also need proselint and mdspell. To get proselint you also need pip.

Lots of dependencies which we know Travis doesn't really like (and would add quite a lot of time to builds until caching is fully implemented #757 (https://github.com/Instagram/IGListKit/issues/757))

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/Instagram/IGListKit/issues/394#issuecomment-303860059), or mute the thread (https://github.com/notifications/unsubscribe-auth/ADOz3dxLpNB4e6oMqbdGuq00K1QznRn8ks5r9KQ6gaJpZM4Ldeac).

[orta]

Yeah - there wasn't anything like proselint or mdspell in ruby, shame :)