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

Fix: [Github] Multiple Inline Comments on the same file/line should all be posted #1176

Merged
merged 4 commits into from
Jan 7, 2022

Conversation

Rouby
Copy link
Contributor

@Rouby Rouby commented Nov 19, 2021

This PR should address #1134 by sending all inline comments as a review combined.

@orta
Copy link
Member

orta commented Nov 19, 2021

Structurally this looks good 👍🏻 - I'd like a test or two though because I don't want this breaking from other contributors changes in the future

@Rouby
Copy link
Contributor Author

Rouby commented Nov 19, 2021

a test or two

Will do :)

Signed-off-by: Jonathan Burke <jonathan.burke@codecentric.de>
Copy link
Member

@fbartho fbartho left a comment

Choose a reason for hiding this comment

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

This PR looks good to me! Just need to add an entry to the Changelog!

Something like the following inside "Your comment below here":

- Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby]

@Rouby
Copy link
Contributor Author

Rouby commented Jan 7, 2022

This PR looks good to me! Just need to add an entry to the Changelog!

Something like the following inside "Your comment below here":

- Fix: [Github] Multiple Inline Comments on the same file/line should all be posted [#1176](https://github.com/danger/danger-js/pull/1176) [@Rouby]

Thanks and done 👍

@fbartho fbartho changed the title extend github api to send complete reviews Fix: [Github] Multiple Inline Comments on the same file/line should all be posted Jan 7, 2022
@fbartho
Copy link
Member

fbartho commented Jan 7, 2022

Note: CI is blocked by #1189 or an alternative solution there (so don’t be alarmed by the test failure)

@f-meloni
Copy link
Member

Would make sense to have the new comment logic enabled when there is a specific flag (e.g., --use-github-reviews) or something like that, so that is disabled by default and we can then investigate it with calm (but Danger keeps working as expected in the meantime)?

@orta
Copy link
Member

orta commented Apr 14, 2022

Yeah, perhaps I misinterpreted this PR, IMO warnings/fails/etc which cannot be registered in the diff should show up in the main comment. I personally find reviews annoying in PRs (because they don't get overwritten by the edits and thus spam the PR on every CI run )

I think this behavior as is could be a flag but we should get it aligned with how danger ruby does is which is ^

@f-meloni
Copy link
Member

Yeah, mine was more a temporary suggestion.
Failed tests or lint issues might be affected by this change, so I thought we could keep those working while this gets solved

@f-meloni
Copy link
Member

PS. I just realised I posted this in the PR and not in the issue 😅 sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants