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

Hk issue 776 #777

Merged
merged 5 commits into from
Nov 27, 2018
Merged
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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
- Update comment instead deleting, if it has replies (BitBucket Server) [@langovoi][]
- Fix BitBucket Server GitDSL [@langovoi][]
- Add support of paged APIs of BitBucket Server [@langovoi][]
- Revert removal of implicit `<p>` tag from [danger/danger-js#754](https://github.com/danger/danger-js/pull/754) and add
disctinction depending on containing markdown or not - [@hanneskaeufler][]

# 6.1.6

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ exports[`generating inline messages Shows correct messages for inline/regular vi
</thead>
<tbody><tr>
<td>:warning:</td>
<td>**File.swift#L10** - Test message</td>
<td>

**File.swift#L10** - Test message
</td>
</tr>

<tr>
Expand All @@ -61,7 +64,40 @@ exports[`generating inline messages Shows correct messages for inline/regular vi
"
`;

exports[`generating messages leaves space between <td>s to allow GitHub to render message content as markdown 1`] = `
exports[`generating messages avoids adding space inside the <td> for proper vertical alignment if the message does not contain any markdown 1`] = `
"
<!--
0 failure:
0 warning:
1 messages

DangerID: danger-id-example-id;
-->



<table>
<thead>
<tr>
<th width=\\"50\\"></th>
<th width=\\"100%\\" data-danger-table=\\"true\\">Messages</th>
</tr>
</thead>
<tbody><tr>
<td>:book:</td>
<td>no markdown here</td>
</tr>
</tbody>
</table>


<p align=\\"right\\">
Generated by :no_entry_sign: <a href=\\"http://github.com/danger/danger-js/\\">dangerJS</a>
</p>
"
`;

exports[`generating messages leaves space between <td>s to allow GitHub to render message content as markdown if the message contains any 1`] = `
"
<!--
1 failure: **Failure:** Some...
Expand All @@ -80,7 +116,10 @@ exports[`generating messages leaves space between <td>s to allow GitHub to rende
</thead>
<tbody><tr>
<td>:no_entry_sign:</td>
<td>**Failure:** Something failed!</td>
<td>

**Failure:** Something failed!
</td>
</tr>
</tbody>
</table>
Expand All @@ -95,7 +134,10 @@ exports[`generating messages leaves space between <td>s to allow GitHub to rende
</thead>
<tbody><tr>
<td>:warning:</td>
<td>_Maybe you meant to run \`yarn install\`?_</td>
<td>

_Maybe you meant to run \`yarn install\`?_
</td>
</tr>
</tbody>
</table>
Expand All @@ -110,11 +152,14 @@ exports[`generating messages leaves space between <td>s to allow GitHub to rende
</thead>
<tbody><tr>
<td>:book:</td>
<td>\`\`\`ts
<td>

\`\`\`ts
function add(a: number, b: number): number {
return a + b
}
\`\`\`</td>
\`\`\`
</td>
</tr>
</tbody>
</table>
Expand Down
13 changes: 12 additions & 1 deletion source/runner/templates/_tests/_githubIssueTemplates.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe("generating messages", () => {
expect(issues).toContain(expected)
})

it("leaves space between <td>s to allow GitHub to render message content as markdown", () => {
it("leaves space between <td>s to allow GitHub to render message content as markdown if the message contains any", () => {
const issues = githubResultsTemplate("example-id", {
fails: [{ message: "**Failure:** Something failed!" }],
warnings: [{ message: "_Maybe you meant to run `yarn install`?_" }],
Expand All @@ -70,6 +70,17 @@ describe("generating messages", () => {

expect(issues).toMatchSnapshot()
})

it("avoids adding space inside the <td> for proper vertical alignment if the message does not contain any markdown", () => {
const issues = githubResultsTemplate("example-id", {
fails: [],
warnings: [],
messages: [{ message: "no markdown here" }],
markdowns: [],
})

expect(issues).toMatchSnapshot()
})
})

describe("generating inline messages", () => {
Expand Down
33 changes: 24 additions & 9 deletions source/runner/templates/githubIssueTemplate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Violation, isInline } from "../../dsl/Violation"
* @returns {string} HTML
*/
function table(name: string, emoji: string, violations: Violation[]): string {
if (violations.length === 0 || violations.every(violation => !violation.message)) {
if (noViolationsOrAllOfThemEmpty(violations)) {
return ""
}
return `
Expand All @@ -23,18 +23,33 @@ function table(name: string, emoji: string, violations: Violation[]): string {
<th width="100%" data-danger-table="true">${name}</th>
</tr>
</thead>
<tbody>${violations
.map((v: Violation) => {
const message = isInline(v) ? `**${v.file!}#L${v.line!}** - ${v.message}` : v.message
return `<tr>
<tbody>${violations.map(violation => htmlForValidation(emoji, violation)).join("\n")}</tbody>
</table>
`
}

function htmlForValidation(emoji: string, violation: Violation) {
let message = isInline(violation)
? `**${violation.file!}#L${violation.line!}** - ${violation.message}`
: violation.message

if (containsMarkdown(message)) {
message = `\n\n ${message}\n `
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 kinda keeps some weird indentation to keep the snapshots happy. Was easier to avoid breaking anything this way.

}

return `<tr>
<td>:${emoji}:</td>
<td>${message}</td>
</tr>
`
})
.join("\n")}</tbody>
</table>
`
}

function containsMarkdown(message: string): boolean {
return message.match(/[`*_~\[]+/g) ? true : false
Copy link
Member Author

Choose a reason for hiding this comment

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

That's pretty naive 🤷🏼‍♂️

}

function noViolationsOrAllOfThemEmpty(violations: Violation[]) {
return violations.length === 0 || violations.every(violation => !violation.message)
}

function getSummary(label: string, violations: Violation[]): string {
Expand Down