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

Support inline comments #77

Open
macklinu opened this Issue Jan 3, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@macklinu
Member

macklinu commented Jan 3, 2017

Danger-rb's methods support the following parameters:

warn (message: String, sticky=false: Boolean, file=nil: String, line=nil: String)

These methods allow for adding a warning/error/messages inline on a specific file. What would be involved in porting that functionality to Danger-js?

The current type definition of warn in Danger-js is:

warn(message: MarkdownString): void

Could we just add more parameters? Would we accept an options object as a second parameter?

// perhaps this
warn(message: MarkdownString, file=undefined: String, line=undefined: String)

// or
warn(message: MarkdownString, options: { file: String, line: String })

Also, where would sticky: Boolean come into play - would that be a part of this work, or should that be a part of a separate issue?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jan 3, 2017

Member

So, from my perspective, I think sticky is something we should avoid. Every Dangerfile that we have at Artsy had sticky turned off on each trigger, which is why I turned it off by default.

Unless someone really, really, really wants it, we should avoid it. It adds a considerable amount of internal complexity (finding all pre-existing violations in every comment / issue before incrementally updating/removing them) on Danger for an occasionally useful feature.

So, file: line:

What would be involved in porting that functionality to Danger-js?

Both of those would work (and be backwards compatible) I think I'm in favour of the options hash. Gives more room for further improvement in the future without making a lot of args on those functions.

Generally the usage of file: line: has been inside danger-rb plugins, where elegance of the code is less important ( you want lots of 2-3 LOC rules ideally )

Implementation:

I see two options here, you can start a review, and make the comments inside there ( see this discussion on what that could look like ) or just outright use the commit API.

Either way, there needs to be

  • A "main violations" abstraction that posts the normal GitHub comment (what we have right now)
  • A "file violation" abstraction that handlesfile: line: -> PR Diff positioning system

This is by far the most gnarly code in Danger-rb, so I advise taking your time on it and trying to find better abstractions than we have in ruby.

Member

orta commented Jan 3, 2017

So, from my perspective, I think sticky is something we should avoid. Every Dangerfile that we have at Artsy had sticky turned off on each trigger, which is why I turned it off by default.

Unless someone really, really, really wants it, we should avoid it. It adds a considerable amount of internal complexity (finding all pre-existing violations in every comment / issue before incrementally updating/removing them) on Danger for an occasionally useful feature.

So, file: line:

What would be involved in porting that functionality to Danger-js?

Both of those would work (and be backwards compatible) I think I'm in favour of the options hash. Gives more room for further improvement in the future without making a lot of args on those functions.

Generally the usage of file: line: has been inside danger-rb plugins, where elegance of the code is less important ( you want lots of 2-3 LOC rules ideally )

Implementation:

I see two options here, you can start a review, and make the comments inside there ( see this discussion on what that could look like ) or just outright use the commit API.

Either way, there needs to be

  • A "main violations" abstraction that posts the normal GitHub comment (what we have right now)
  • A "file violation" abstraction that handlesfile: line: -> PR Diff positioning system

This is by far the most gnarly code in Danger-rb, so I advise taking your time on it and trying to find better abstractions than we have in ruby.

@macklinu

This comment has been minimized.

Show comment
Hide comment
@macklinu

macklinu Jan 5, 2017

Member

Regarding danger/danger#684, I'm thinking adding a GitHub review feature could be separate from just adding a { file, line } options hash to the warn/message/fail/markdown functions. I can see value in making one-off comments using Danger as well as integrating into GitHub's review feature. If Danger-rb supports both (or is in the process of doing so), Danger-js can support the same API for parity between tools (we can leave sticky out of Danger-js, though 😃).

Thanks for posting links and explaining the PR Diff positioning system - they will be useful for understand how to parse the diff. A coworker went through a similar experience trying to parse GitHub diffs for an Android static analysis tool he and another coworker have been working on, so I think those could also be some helpful references.

I'm thinking the diff parser could be added to this repo, and if generic enough, it could be pulled out into an npm package for reuse - I didn't see anything when searching npmjs.com that did quite what we need here.

Member

macklinu commented Jan 5, 2017

Regarding danger/danger#684, I'm thinking adding a GitHub review feature could be separate from just adding a { file, line } options hash to the warn/message/fail/markdown functions. I can see value in making one-off comments using Danger as well as integrating into GitHub's review feature. If Danger-rb supports both (or is in the process of doing so), Danger-js can support the same API for parity between tools (we can leave sticky out of Danger-js, though 😃).

Thanks for posting links and explaining the PR Diff positioning system - they will be useful for understand how to parse the diff. A coworker went through a similar experience trying to parse GitHub diffs for an Android static analysis tool he and another coworker have been working on, so I think those could also be some helpful references.

I'm thinking the diff parser could be added to this repo, and if generic enough, it could be pulled out into an npm package for reuse - I didn't see anything when searching npmjs.com that did quite what we need here.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jan 6, 2017

Member

We do already have a diff parser in Danger JS - here but what I mean is WRT the diff parser is: You need to take an absolute file path + line, e.g. { file: "CHANGELOG.md", line: 6 } and come back with something that could be found inside the existing diff object that we have e.g. { commit: 918b1dc, file: "CHANGELOG.md", line: 9} or if it wasn't found, then it needs to go inside the main comment.
The API docs will give you a sense of what I mean

Other than that, all sounds fine to me 👍

Member

orta commented Jan 6, 2017

We do already have a diff parser in Danger JS - here but what I mean is WRT the diff parser is: You need to take an absolute file path + line, e.g. { file: "CHANGELOG.md", line: 6 } and come back with something that could be found inside the existing diff object that we have e.g. { commit: 918b1dc, file: "CHANGELOG.md", line: 9} or if it wasn't found, then it needs to go inside the main comment.
The API docs will give you a sense of what I mean

Other than that, all sounds fine to me 👍

@macklinu

This comment has been minimized.

Show comment
Hide comment
@macklinu

macklinu Jan 6, 2017

Member

Thanks @orta, think I misunderstood about the diff parsing thing originally. I'll dive into the code and get my hands dirty. Will open a PR if I have any more questions. 😄

Member

macklinu commented Jan 6, 2017

Thanks @orta, think I misunderstood about the diff parsing thing originally. I'll dive into the code and get my hands dirty. Will open a PR if I have any more questions. 😄

@macklinu

This comment has been minimized.

Show comment
Hide comment
@macklinu

macklinu Mar 21, 2017

Member

This issue is open if someone would like to contribute! An initial attempt to solve this was in #99, which might give a starting point. Feel free to comment here to ask more questions or open a PR to get work-in-progress feedback. 😃

Member

macklinu commented Mar 21, 2017

This issue is open if someone would like to contribute! An initial attempt to solve this was in #99, which might give a starting point. Feel free to comment here to ask more questions or open a PR to get work-in-progress feedback. 😃

@tychota

This comment has been minimized.

Show comment
Hide comment
@tychota

tychota Aug 21, 2017

Contributor

I would be pleased to help on this.

At www.bam.tech, we want to use danger as a poka-yoke, to share good practice and learning accros the teams. Inline comment have a big impact on it.

That being said, I have a bad overview of danger js, so maybe it is to complicated for a first task.

My plan so far:

  • backport MessagingOptions from #99
  • access danger context to get the diff:
    • if the inline comment is in the diff, add the inline object to a list of object and the a summary of inline comment (necessary ?)
    • else do the details summary thing (dear-github/dear-github#166 (comment)) inside the githubResultsTemplate
  • if the inline object, implement a review call and inline feedback
  • add a setting (env variables/ api) to fallback to use the commit api if the user do not want a review

Does it seems a good plan for you ?

What are the difficult points / points to carefully design the abstraction ?

Contributor

tychota commented Aug 21, 2017

I would be pleased to help on this.

At www.bam.tech, we want to use danger as a poka-yoke, to share good practice and learning accros the teams. Inline comment have a big impact on it.

That being said, I have a bad overview of danger js, so maybe it is to complicated for a first task.

My plan so far:

  • backport MessagingOptions from #99
  • access danger context to get the diff:
    • if the inline comment is in the diff, add the inline object to a list of object and the a summary of inline comment (necessary ?)
    • else do the details summary thing (dear-github/dear-github#166 (comment)) inside the githubResultsTemplate
  • if the inline object, implement a review call and inline feedback
  • add a setting (env variables/ api) to fallback to use the commit api if the user do not want a review

Does it seems a good plan for you ?

What are the difficult points / points to carefully design the abstraction ?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Aug 21, 2017

Member

That feels about right, yeah 👍

Here's the main chunk of the work in danger ruby.

I've slowly grown to the idea that I want to see something like #196 which will end up requiring the "did this danger instance create this table" but that's further down the line. Most of the work is in deciding where a comment should go, and making sure every comment is up-to-date when danger does a second run

Member

orta commented Aug 21, 2017

That feels about right, yeah 👍

Here's the main chunk of the work in danger ruby.

I've slowly grown to the idea that I want to see something like #196 which will end up requiring the "did this danger instance create this table" but that's further down the line. Most of the work is in deciding where a comment should go, and making sure every comment is up-to-date when danger does a second run

@macklinu

This comment has been minimized.

Show comment
Hide comment
@macklinu

macklinu Aug 21, 2017

Member

Thanks for looking into this, @tychota!

add a setting (env variables/ api) to fallback to use the commit api if the user do not want a review

If it's possible to weave GitHub reviews and inline comments together through some API configuration option, that sounds awesome. I guessed that inline commenting would be the simplest first step, but I'm not familiar with GitHub's API on either of these things, so I'm not sure what would be doable in one PR.

For reference, here is the Ruby PR where the GitHub reviews feature was introduced in Danger - danger/danger#702.

Member

macklinu commented Aug 21, 2017

Thanks for looking into this, @tychota!

add a setting (env variables/ api) to fallback to use the commit api if the user do not want a review

If it's possible to weave GitHub reviews and inline comments together through some API configuration option, that sounds awesome. I guessed that inline commenting would be the simplest first step, but I'm not familiar with GitHub's API on either of these things, so I'm not sure what would be doable in one PR.

For reference, here is the Ruby PR where the GitHub reviews feature was introduced in Danger - danger/danger#702.

@tychota

This comment has been minimized.

Show comment
Hide comment
@tychota

tychota Aug 27, 2017

Contributor

@macklinu @orta: until September 6, I have a bit of vacation and two talks for meetups/training to prepare so it will be pretty busy.

I would definitively have more time after that and your links is so valuable to me. I have now a clearer idea about the work required.

Contributor

tychota commented Aug 27, 2017

@macklinu @orta: until September 6, I have a bit of vacation and two talks for meetups/training to prepare so it will be pretty busy.

I would definitively have more time after that and your links is so valuable to me. I have now a clearer idea about the work required.

@Rush

This comment has been minimized.

Show comment
Hide comment
@Rush

Rush Mar 5, 2018

Any progress on this?

Rush commented Mar 5, 2018

Any progress on this?

@tivac

This comment has been minimized.

Show comment
Hide comment

tivac commented Mar 6, 2018

@tychota

This comment has been minimized.

Show comment
Hide comment
@tychota

tychota Mar 6, 2018

Contributor

Wow.

Contributor

tychota commented Mar 6, 2018

Wow.

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