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. #529

Merged
merged 41 commits into from Mar 31, 2018

Conversation

Projects
None yet
4 participants
@sunshinejr
Member

sunshinejr commented Mar 4, 2018

(Because this is basing off of #516, I'm currently targeting it's branch to see actual changes, when it's merged I'm gonna switch it back)

Live demo: Harvey#17

Todo:

  • Proof of Concept
  • Position finder for GitHub
  • Send inline comment in the main comment when we can't make an inline one.
  • Make one inline comment for all violations in a table
  • Merge BitBucket support from danger-js and depend on it
  • Improve inline comment
  • Sort inline comments
  • Main comment template should include file/line if violation is just out of scope of the PR
  • Update inline comments after next commit if the body changed
  • Delete comment when it's no longer needed (after commit fixes)
  • Revisit getting PR chunks to post an inline comment
  • Add documentation
  • Add Changelog entry

@sunshinejr sunshinejr referenced this pull request Mar 4, 2018

Merged

Inline mode #56

2 of 2 tasks complete

@sunshinejr sunshinejr referenced this pull request Mar 5, 2018

Closed

[DON'T MERGE] Danger test. #17

3 of 3 tasks complete
const dslFiles = fs.readdirSync("source/dsl").map(f => `source/dsl/${f}`)
const dslFiles = fs
.readdirSync("source/dsl")
.filter(f => !f.startsWith("_tests"))

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

nice call

@orta

Great progress

/** Git diff chunks */
// Not sure if this is the right place for this one, but few methods needs both chunks & text diffs and I didn't want to parse two times the same file
// We could rename the interface, but couldn't find a good one
chunks: any[]

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

interesting, I wonder if the object still has the same shape with the bitbucket support

}
export function isInline(violation: Violation): boolean {
return violation.file !== undefined || violation.line !== undefined

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

I wonder if outright enforcing both is probably the right call here, then you can make safe assumptions inside the inline side of the code, which is already gonna be complex

This comment has been minimized.

@sunshinejr

sunshinejr Mar 6, 2018

Member

I agree, this is like the first line I wrote and after the whole PR I definitely think this is a right call. Gonna fix.

This comment has been minimized.

@sunshinejr
/**
* Adds a message to the Danger table, the only difference between this
* and warn is the emoji which shows in the table.
*
* @param {MarkdownString} message the String to output
*/
function message(message: MarkdownString): void
function message(message: MarkdownString, file?: string, line?: number): void

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

any chance you can add the @params - something like:

    * @param {MarkdownString} message the String to output
    * @param {number | undefined} file a file which this message should be attached to
    * @param {number | undefined} line the line which this message should be attached to

This comment has been minimized.

@sunshinejr
@@ -26,7 +25,7 @@ export interface DangerResults {
/**
* Markdown messages to attach at the bottom of the comment
*/
markdowns: MarkdownString[]
markdowns: Violation[]

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

yeah, I should have done that from the start - thanks

}
}
}
Object.keys(results).forEach(violationsIntoInlineResults)

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

does this do something?

This comment has been minimized.

@sunshinejr

sunshinejr Mar 6, 2018

Member

Oh, yeah. So this line does iterate through results keys (warnings, fails, messages, markdowns) and for each kind it generates violations for inline results and appends to a variable initialized before. This whole function is a tough one, I needed to transform DangerResults into DangerInlineResults[] and I couldn't find a nicer way. Basically a transformation from:

{
  fails: Violation[],
  warnings: Violation[],
  messages: Violation[],
  markdowns: Violation[]
}

to

{
  file: string,
  line: number,
  fails: string[],
  warnings: string[],
  messages: string[],
  markdowns: string[]
}

I'm all ears if you have something better-looking in mind 😅

This comment has been minimized.

@orta

orta Mar 6, 2018

Member

Ah, I guess you have to mutate the results object? Yeah, that's a bit of a toughie, maybe it just needs a comment saying what's happening

This comment has been minimized.

@sunshinejr

sunshinejr Mar 6, 2018

Member

Added a comment - let me know how it looks!

This comment has been minimized.

@orta

orta Mar 6, 2018

Member

Yep, that's good

const inline = inlineResults(results)
const inlineLeftovers = await this.sendInlineComments(inline, git)
const regular = regularResults(results)
const mergedResults = sortResults(mergeResults(regular, inlineLeftovers))

This comment has been minimized.

@orta

orta Mar 5, 2018

Member

great work on handling the leftovers, thought I would have this bring this up 👍

@tivac tivac referenced this pull request Mar 6, 2018

Closed

Support inline comments #77

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Mar 6, 2018

Also, @orta, I would need an advice on what should be a default behavior for inline comments after each commit. Should we delete all of them and create new ones in it's place? Should we update existing ones? Should we only remove the ones that are not present in the newest commit?

Oh, and how about the template that we use for inline comments? Right now I'm using the one for main comment, but it seems kinda stacked (maybe remove headers?)

@orta

This comment has been minimized.

Member

orta commented Mar 6, 2018

I'm not too worried about the template for the inlines, every time I've seen someone use it, it's been with markdown so they get full control.

However, a smaller version could just be a markdown list instead:

  • 🚫 Jk
  • 🚫 Fix it bro
  • ⚠️ Bad empty line!
<!--
  2 failures:  Jk, fix it bro
  1 warning:  Bad empty line!
  1 messages
  
  DangerID: danger-id-default;
-->

- :no_entry_sign: Jk
- :no_entry_sign: Fix it bro
- :warning: Bad empty line!

I'd be fine with it skipping the "posted by danger" too. So you could definitely improve that.

@orta

This comment has been minimized.

Member

orta commented Mar 6, 2018

Re: comments, yeah, ideally edit, I know it's a bunch of work but it triggers a lot of emails when comments are deleted and re-created.

Perhaps you could extend the current danger-id to include file and line per message. This means lookups in the existing comments list should be pretty simple:

  • you can mark all of the inline comments as to be deleted
  • then as you edit a comment it gets removed from that list
  • once all danger results have been handled, delete the comments that haven't been edited
@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Mar 6, 2018

@orta that's a great point, didn't think about the emails. It's gonna take more time, but I think it's worth the cause.

Oh, and found a few secs to update the inline comment template with tests. Simple bullet points are even better than a table with rows, should look much cleaner.

Edit: Forgot about BitbucketServer comments, gonna add them as well in a few (even though we won't support it in the beginning, no problem adding a core prepared for implementing api endpoints)

Edit2: BitbucketServer inline comments template added as well

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Mar 18, 2018

@orta I think I’m kinda satisfied with it, should be ready for the final check. I refactored chunks from TextDiff, so it has a separate function to get it now (structuredDiffForFile).

The only thing I’m not sure about is that passing gitDSL around. Runner -> Executor -> Platform -> GitHub all have it just to get a diff on a file (right now also a commit hash, but it can be fetched from a platform I believe). If we could somehow get a gitDSL (or just chunks for each fil) in platforms(GitHub and now BitBucket), it should be all cleaner. But it can be refactored later of course.

I would also love some help with docs, not sure where to even start 😅

Disclaimer: I haven't fully tested the latest version on Harvey, as we have a problem with CircleCI right now, thus I would wait a bit before a final merge anyways.

Update: I've tested it on Harvey, as the problem was fixed a few minutes ago, and it seems like removing/updating comments is working correctly! 🎉

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

@orta

This comment has been minimized.

Member

orta commented Mar 20, 2018

OK, this is cool - I'll handle all the docs side, but I'm on a deadline (for tomorrow) so will take a serious look at the weekend and update this PR. 👍

Awesome

@orta

orta approved these changes Mar 25, 2018

let processName = subprocessName[0]
let args = subprocessName
let results = {} as any
args.shift() // mutate and remove the first element
const processDisplayName = path.basename(processName)
const dslJSONString = prepareDangerDSL(dslJSON)
d(`Running subprocess: ${processDisplayName} - ${args}`)

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

cool, - this makes sense!

markdowns: string[]
}
export const emptyDangerResults = {

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

These are in the danger.d.ts - will need to remove these for prod

const prID = this.repoMetadata.pullRequestID
return await this.getAllOfResource(`repos/${repo}/pulls/${prID}/comments`).then(v => {
return v.map((i: any) => {
return { id: i.id, ownedByDanger: i.user.id == userID, body: i.body }

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

I think this will want to also have a danger-id check (could be reasonable to imagine many dangers posting inline)

@@ -162,7 +181,8 @@ export class Executor {
*
* @param {DangerResults} results a JSON representation of the end-state for a Danger run
*/
async handleResultsPostingToPlatform(results: DangerResults) {
// TODO: Instead of danger, pass gitDSL

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

this got did - I think?

resolve(dangerResults.reduce((acc, r) => mergeResults(acc, r), emptyDangerResults))
})
})
}

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

this is some 👍 code

expect(platform.updateInlineComment).not.toBeCalled()
expect(platform.createInlineComment).toHaveBeenCalledTimes(2)
expect(platform.deleteInlineComment).toHaveBeenCalledTimes(2)
})

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

solid tests

${results.messages.map(printViolation("book"))}
${results.markdowns.map(v => v.message).join("\n\n")}
`
}

This comment has been minimized.

@orta

orta Mar 25, 2018

Member

Yep

orta added some commits Mar 26, 2018

@orta

This comment has been minimized.

Member

orta commented Mar 26, 2018

Conceptually, this is basically good to go 👍 ! Code-wise and architecture-wise I'm happy, but I've run out of time this evening and haven't managed to successfully get an inline message to appear. So good in theory, but doesn't look to be fully hooked up in practice yet.

I'm reasonably sure that my changes won't have broke it anyway 👯‍♂️

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Mar 27, 2018

Okay, I'm investigating. Added logging to the PR, gonna remove it after the investigation.

Gonna need to figure out where is the Danger log, though 😅 Okay the log for Danger is for sure in the Node.js:node build on Travis, so I'm gonna use this one.

  • Seems like the structuredDiffForFile returns NULL, so the diffForFile would do as well. That's why we couldn't find a position for inline comment and that's why the inline comments are in the main comment (our fallback for inline comments that are not a part of PR). Now we need to know why we don't have a diff 🤔
  • Seems like Danger executes wrong URL for PR (diff):
    https://api.github.com/repos/sunshinejr/danger-js/pulls/529
    instead of
    https://api.github.com/repos/danger/danger-js/pulls/529
  • TRAVIS_PULL_REQUEST_SLUG might be the answer (instead of TRAVIS_REPO_SLUG), let's see. From the docs TRAVIS_PULL_REQUEST_SLUG is empty for a push build, not a PR build, so it's not that safe - we might want to use TRAVIS_PULL_REQUEST_SLUG when possible and switch to TRAVIS_REPO_SLUG otherwise. TRAVIS_REPO_SLUG is fine, investigating further.
  • Okay, seems like down there somewhere there is no repoSlug and then it is created by github.pr.head.repo.full_name. This is always sunshinejr/danger-js, not sure where it's needed for now, but we need danger/danger-js for getting diff. There are 3 places that use this repo name. Interesting though.
  • Good news, I think I've fixed it, inline comments are posted, main comment updated, just need to update 27 tests now 😅 (probably will do it tomorrow, though of course I did it today 😄)

@orta found the issue, fixed it, fixed tests. Basically this one. When creating a new GithubAPI we were passing a repoSlug that is a PRs repo, not a base repo, thus we couldn't get a diff using the wrong URL. I'm still wondering why other methods did have a good repoSlug... 🤔

@orta

This comment has been minimized.

Member

orta commented Mar 31, 2018

Yeah, the case of different repo is particularly common for my projects as I nearly always do same-repo PRs 👍

@orta

This comment has been minimized.

Member

orta commented Mar 31, 2018

Going to give it one more audit, then I'll get this shipped

orta added some commits Mar 31, 2018

@orta

orta approved these changes Mar 31, 2018

It's happening.

Great work @sunshinejr - this is a tonne of work and hard thinking.

🌞 👶

@orta orta merged commit 0c7e7d7 into danger:master Mar 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@orta

This comment has been minimized.

Member

orta commented Mar 31, 2018

Heh, this sneaked in an extra root export:

screen shot 2018-03-31 at 10 34 32 am

Fixed with a49c379

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

@sunshinejr

This comment has been minimized.

Member

sunshinejr commented Mar 31, 2018

@orta I just figured out what you meant with the emoji and it's awesome 😄

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