Skip to content
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

[WIP] Support inline comments #99

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions source/dsl/Aliases.ts
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export type MarkdownString = string
export type Filename = string
export type LineNumber = string
5 changes: 2 additions & 3 deletions source/dsl/DangerResults.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Violation } from "../platforms/messaging/violation"
import { MarkdownString } from "../dsl/Aliases"

/**
* Representation of what running a Dangerfile generates.
Expand Down Expand Up @@ -27,7 +26,7 @@ export interface DangerResults {

/**
* Markdown messages at the bottom of the comment
* @type {MarkdownString[]}
* @type {Violation[]}
*/
markdowns: Array<MarkdownString>
markdowns: Array<Violation>
}
4 changes: 4 additions & 0 deletions source/platforms/messaging/violation.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { MessagingOptions } from "../../runner/Dangerfile"

/**
* The result of user doing warn, message or fail.
*/
Expand All @@ -8,4 +10,6 @@ export interface Violation {
* @type {string}
*/
message: string

options?: MessagingOptions
}
23 changes: 14 additions & 9 deletions source/runner/Dangerfile.ts
Original file line number Diff line number Diff line change
@@ -1,35 +1,35 @@
import { DangerResults } from "../dsl/DangerResults"
import { DangerDSLType } from "../dsl/DangerDSL"
import { MarkdownString } from "../dsl/Aliases"
import { MarkdownString, Filename, LineNumber } from "../dsl/Aliases"

export interface DangerContext {
/**
* Fails a build, outputting a specific reason for failing
*
* @param {MarkdownString} message the String to output
*/
fail(message: MarkdownString): void
fail(message: MarkdownString, options?: MessagingOptions): void

/**
* Highlights low-priority issues, does not fail the build
*
* @param {MarkdownString} message the String to output
*/
warn(message: MarkdownString): void
warn(message: MarkdownString, options?: MessagingOptions): void

/**
* Puts a message inside the Danger table
*
* @param {MarkdownString} message the String to output
*/
message(message: MarkdownString): void
message(message: MarkdownString, options?: MessagingOptions): void

/**
* Puts a message inside the Danger table
*
* @param {MarkdownString} message the String to output
*/
markdown(message: MarkdownString): void
markdown(message: MarkdownString, options?: MessagingOptions): void

/** Typical console */
console: Console
Expand All @@ -48,6 +48,11 @@ export interface DangerContext {
results: DangerResults
}

export interface MessagingOptions {
file?: Filename
line?: LineNumber
}

/** Creates a Danger context, this provides all of the global functions
* which are available to the Danger eval runtime.
*
Expand All @@ -62,10 +67,10 @@ export function contextForDanger(dsl: DangerDSLType): DangerContext {
markdowns: []
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

results.markdowns: Array differs in that it currently accepts MarkdownString as its type, whereas fails, warns, and messages accept Violation as its type. To keep parity with the Danger-rb API, markdown() should be able to accept { file, line } options, so I believe moving markdowns from Array<MarkdownString> to Array<Violation> is the appropriate change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this makes sense 👍


const fail = (message: MarkdownString) => results.fails.push({ message })
const warn = (message: MarkdownString) => results.warnings.push({ message })
const message = (message: MarkdownString) => results.messages.push({ message })
const markdown = (message: MarkdownString) => results.markdowns.push(message)
const fail = (message: MarkdownString, options: MessagingOptions = {}) => results.fails.push({ message, options })
const warn = (message: MarkdownString, options: MessagingOptions = {}) => results.warnings.push({ message, options })
const message = (message: MarkdownString, options: MessagingOptions = {}) => results.messages.push({ message, options })
const markdown = (message: MarkdownString, options: MessagingOptions = {}) => results.markdowns.push({ message, options })

return {
fail,
Expand Down
3 changes: 2 additions & 1 deletion source/runner/Executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ export class Executor {
process.exitCode = 1
}

// Delete the message if there's nothing to say
const {fails, warnings, messages, markdowns} = results

const failureCount = [...fails, ...warnings].length
Expand All @@ -75,6 +74,7 @@ export class Executor {
this.d(results)

if (failureCount + messageCount === 0) {
// Delete the message if there's nothing to say
console.log("No messages are collected.")
await this.platform.deleteMainComment()
} else {
Expand All @@ -84,6 +84,7 @@ export class Executor {
console.log("Found some message, writing it down")
}
const comment = githubResultsTemplate(results)
// TODO filter out Danger items that contain file/line info
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the area where we could filter out results that contained a complete { file, line } object and make inline comments. if { file, line } is incomplete/missing, add them to the main Danger comment (I think this refers to your abstractions comment in #77 (comment), @orta).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is where you want to find:

  • Things with no file/line (main comment)
  • Things with file/line inside the diff (inline)
  • Things with file/line outside the diff (main comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function only accepts the results: DangerResults. I'm thinking we will need the context: DangerContext (or at least danger: DangerDSLType), so that we can call danger.git.diffForFile(violation.options.file) and determine if the { file, line } supplied are valid for creating an inline comment. Does that make sense?

await this.platform.updateOrCreateComment(comment)
}
}
Expand Down
8 changes: 4 additions & 4 deletions source/runner/_tests/DangerRunner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ describe("with fixtures", () => {
const results = await runDangerfileEnvironment(resolve(fixtures, "__DangerfileFullMessages.js"), runtime)

expect(results).toEqual({
fails: [{"message": "this is a failure"}],
markdowns: ["this is a *markdown*"],
messages: [{"message": "this is a message"}],
warnings: [{"message": "this is a warning"}]
fails: [{"message": "this is a failure", "options": {}}],
markdowns: [{"message": "this is a *markdown*", "options": {}}],
messages: [{"message": "this is a message", "options": {}}],
warnings: [{"message": "this is a warning", "options": {}}]
})
})

Expand Down
11 changes: 9 additions & 2 deletions source/runner/_tests/fixtures/ExampleDangerResults.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,16 @@ export const failsResults: DangerResults = {
markdowns: []
}

export const markdownResults: DangerResults = {
fails: [],
warnings: [],
messages: [],
markdowns: [{ message: "Markdown message" }]
}

export const summaryResults: DangerResults = {
fails: [{ message: "Failing message Failing message" }],
warnings: [{ message: "Warning message Warning message" }],
messages: [{ message: "message" }],
markdowns: ["markdown"],
}
markdowns: [{ message: "markdown" }],
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { emptyResults, warnResults, failsResults, summaryResults } from "../../_tests/fixtures/ExampleDangerResults"
import { emptyResults, warnResults, failsResults, summaryResults, markdownResults } from "../../_tests/fixtures/ExampleDangerResults"
import { template as githubResultsTemplate } from "../../templates/github-issue-template"

describe("generating messages", () => {
Expand All @@ -21,6 +21,11 @@ describe("generating messages", () => {
expect(issues).not.toContain("Fails")
})

it("shows the markdown messages separately from results table", () => {
const issues = githubResultsTemplate(markdownResults)
expect(issues).toContain("Markdown message")
})

it("does not break commonmark rules around line breaks", () => {
const issues = githubResultsTemplate(warnResults)
expect(issues).not.toMatch(/(\r?\n){2}[ \t]+</)
Expand Down
2 changes: 1 addition & 1 deletion source/runner/templates/github-issue-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ ${buildSummaryMessage(results)}
${table("Fails", "no_entry_sign", results.fails)}
${table("Warnings", "warning", results.warnings)}
${table("Messages", "book", results.messages)}
${results.markdowns.join("\n\n")}
${results.markdowns.map(markdown => markdown.message).join("\n\n")}
<p align="right">
Generated by :no_entry_sign: <a href="http://github.com/danger/danger-js/">dangerJS</a>
</p>
Expand Down