-
-
Notifications
You must be signed in to change notification settings - Fork 49
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 Dangerfile example #41
Conversation
README.md
Outdated
warn("Big PR, try to keep changes smaller if you can") | ||
} | ||
|
||
if (danger.github!!.pullRequest.title.contains("WIP" ,false)) { |
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.
.github!!
this something you can do at declaration-ish (out of user-land code) point? (like how in Swift/TS it pretends to not be be nullable)
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 didn't find a way to force unwrap on the type declaration, and Moshi
(the library that we are using to decode the JSON) requests an optional for this.
A valid alternative could be make this private and expose a getter that force unwraps it
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 opened #42, let's see if fixes this
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.
approved even if i don't like _varName
🙈
You can make a Dangerfile that looks through PR metadata, it's fully typed. | ||
|
||
```kotlin | ||
import com.danger.dangerkotlin.* |
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.
systems.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.
Yes I suppose is actually better given the website URL :P
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.
happy for systems.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.
but i think we should do it in a separate PR
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.
let's merge everything what we have, then let's go with the migration to the new package name
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.
Yeah, sorry - was just thinking from a high level, no need to rush for it 👍
merge on green |
Fixes #40