[WIP] Support inline comments #99
Conversation
Generated by 🚫 dangerJS |
|
|
||
| /** | ||
| * Puts a message inside the Danger table | ||
| * | ||
| * @param {MarkdownString} message the String to output | ||
| */ | ||
| markdown(message: MarkdownString): void | ||
| markdown(message: MarkdownString, options?: DangerOptions): void |
There was a problem hiding this comment.
markdown() should support { file, line } options too, right?
There was a problem hiding this comment.
I think so, I can see there are good reasons for wanting to put arbitrary markdown on a file 👍
| 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) |
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
| export function template(results: DangerResults): string { | ||
| return ` | ||
| ${table("Fails", "no_entry_sign", results.fails)} | ||
| ${table("Warnings", "warning", results.warnings)} | ||
| ${table("Messages", "book", results.messages)} | ||
| ${results.markdowns.join("\n\n")} | ||
| ${table("Markdowns", "information_source", results.markdowns)} |
There was a problem hiding this comment.
information_source = ℹ️
There was a problem hiding this comment.
Actually, I'm going to change this back to just displaying markdown as it's provided, based on this comment - #109 (comment).
|
|
||
| /** | ||
| * Puts a message inside the Danger table | ||
| * | ||
| * @param {MarkdownString} message the String to output | ||
| */ | ||
| markdown(message: MarkdownString): void | ||
| markdown(message: MarkdownString, options?: DangerOptions): void |
There was a problem hiding this comment.
I think so, I can see there are good reasons for wanting to put arbitrary markdown on a file 👍
| 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) |
| @@ -48,6 +48,11 @@ export interface DangerContext { | |||
| results: DangerResults | |||
| } | |||
|
|
|||
| export interface DangerOptions { | |||
There was a problem hiding this comment.
That's a better name, will update. 👍
| @@ -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 | |||
There was a problem hiding this comment.
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)
| export function template(results: DangerResults): string { | ||
| return ` | ||
| ${table("Fails", "no_entry_sign", results.fails)} | ||
| ${table("Warnings", "warning", results.warnings)} | ||
| ${table("Messages", "book", results.messages)} | ||
| ${results.markdowns.join("\n\n")} | ||
| ${table("Markdowns", "information_source", results.markdowns)} |
s/DangerOptions/MessagingOptions
|
I haven't forgotten about this PR. Been focused on client work recently but should have more time to devote to this feature during the rest of this week/weekend. 👨💻 |
|
Sidenote: here is an amazing example of inlining on to the main comment ( https://twitter.com/tivac/status/821994608984924160 )with syntax highlighting etc |
|
This PR has been sitting here and I haven't gotten a chance to devote serious time to it since early January. I'm going to close this for now and leave #77 open for someone to pick up if they'd like to. I'll comment on the issue if I dive back into it, but this is a desirable feature that I don't want to hold up if someone has more time to work on it. |
|
👍 |
Resolves #77
This is a work-in-progress for #77. I've been sitting on this commit and wanted to get it pushed up and ask some questions about the API and if I'm heading in the right direction before digging in deeper.
TODO:
DangerContext)