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

Inline mode #56

Merged
merged 5 commits into from Mar 31, 2018

Conversation

Projects
None yet
4 participants
@sunshinejr
Member

sunshinejr commented Mar 4, 2018

(This needs to wait before danger-js#529 is merged, though)

Live demo: Harvey#17

Todo:

  • Add documentation
  • Add Changelog entry
@DangerCI

This comment has been minimized.

DangerCI commented Mar 4, 2018

Warnings
⚠️

Any changes to library code should be reflected in the Changelog.

Please consider adding a note there and adhere to the Changelog Guidelines.

⚠️

PR is classed as Work in Progress

Generated by 🚫 dangerJS

@sunshinejr sunshinejr referenced this pull request Mar 4, 2018

Merged

Inline mode #9

2 of 2 tasks complete
/// Adds a warning message to the Danger report
///
/// - Parameter message: A markdown-ish
public func fail(_ message: String) {
DangerRunner.shared.results.fails.append(Violation(message: message))
}
/// Adds an inline fail message to the Danger report
public func fail(message: String, file: String, line: Int) {

This comment has been minimized.

@SD10

SD10 Mar 5, 2018

Member

Any reason you chose to use function overloading instead of just making file & string parameters optional with defaults?

This comment has been minimized.

@sunshinejr

sunshinejr Mar 5, 2018

Member

Oh, yeah. I just don't think we should let users add the file without a line. Although it's possible to do (in danger-js, currently, because I couldn't find a better way without over-engineering), it doesn't do much (will just end up in the main comment anyways).

This comment has been minimized.

@SD10

SD10 Mar 5, 2018

Member

Great point 👍 Was just curious and wanted to understand your reasoning

This comment has been minimized.

@sunshinejr

sunshinejr Mar 5, 2018

Member

Totally, didn't think about that before, good question 👍

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

Yeah, I like this 👍

@sunshinejr sunshinejr referenced this pull request Mar 5, 2018

Closed

[DON'T MERGE] Danger test. #17

3 of 3 tasks complete
@orta

orta approved these changes Mar 5, 2018 edited

Cool yep, nice and simple, makes sense

/// Adds a warning message to the Danger report
///
/// - Parameter message: A markdown-ish
public func fail(_ message: String) {
DangerRunner.shared.results.fails.append(Violation(message: message))
}
/// Adds an inline fail message to the Danger report
public func fail(message: String, file: String, line: Int) {

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

Yeah, I like this 👍

@sunshinejr sunshinejr changed the title from [WIP] Inline mode to Inline mode Mar 24, 2018

@orta

This comment has been minimized.

Member

orta commented Mar 31, 2018

I'll handle the rest

@orta orta merged commit 01f316e into danger:master Mar 31, 2018

1 check passed

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

This comment has been minimized.

Contributor

peril-staging bot commented Mar 31, 2018

Thanks for the PR @sunshinejr.

The Danger org conform to the Moya Community Continuity Guidelines, which means
that we want to offer any contributor the ability to control their destiny.

So, we've sent you an org invite - thanks 🎉

Generated by 🚫 dangerJS

@sunshinejr sunshinejr deleted the sunshinejr:inline branch Mar 31, 2018

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