-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Initial Danger integration #1036
Conversation
I've done this a first step towards #1025. |
Yes! My idea would be having a custom formatter as you hinted and then reading it and commenting with danger. Or we could use the JSON formatter as danger-swiftlint does and format it (with links to the files in their repos for example). We'd need to make danger run in the end though. Or we could have two Dangerfiles - one for PR validation (in this PR), which could be run in any job; and another one that runs after the OSS-check. |
We can have something like this:
Unfortunately, checkboxes don't work inside tables, so we'd need another way to mark a violation as legit. Actually, even if it did danger would delete it on a new build 😬 |
How hard would it be to build our own auto-updating GitHub comment reporting, which would actually allow the format we want? It doesn't seem too challenging, but I may be ignorant of the complexity involved... |
Danger does support any markdown text. Or are you thinking about something else? |
Ah, I see. I thought it was necessary to use the table format. But that still doesn't help us avoid overwriting "approved" violations. |
Yeah, we could get away with that by using GitHub API and applying our own logic in |
Current coverage is 83.15% (diff: 100%)@@ master #1036 diff @@
==========================================
Files 145 145
Lines 7098 7098
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 5902 5902
Misses 1196 1196
Partials 0 0
|
I've created @SwiftLintBot, a new token for that account and added it in Travis's |
Yes, that's the expected behavior. You can run it locally against a PR that would fail to check:
|
markdown <<-MARKDOWN | ||
Here's an example of your CHANGELOG entry: | ||
```markdown | ||
* #{github.pr_title}#{' '} |
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.
Why not this?
-* #{github.pr_title}#{' '}
-[#{github.pr_author}](https://github.com/#{github.pr_author})
-[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)
+* #{github.pr_title}
+ [#{github.pr_author}](https://github.com/#{github.pr_author})
+ [#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)
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.
because I shamelessly copied this from CocoaPods' Dangerfile 😂
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've kept the #{' '}
to make sure we don't miss this if someone uses an editor that strips trailing whitespaces.
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.
that's just one space not two, though?
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.
Just curious, why the two spaces at the end of the github.pr_title
line?
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.
To force a newline.
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.
It was taken from CocoaPods.
@@ -13,6 +13,7 @@ matrix: | |||
env: JOB=SPM | |||
os: osx | |||
osx_image: xcode8.2 | |||
before_script: bundle install --jobs=3 --retry=3 --without documentation --path bundle && bundle exec danger |
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.
maybe break this up into a YAML array for legibility?
before_script:
- bundle install --jobs=3 --retry=3 --without documentation --path bundle
- bundle exec danger
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.
👍
We should probably add |
Fixes #843
@jpsim We need to create a bot account and a token with it, then add it as an env var (
DANGER_GITHUB_API_TOKEN
)From
danger init
: