-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
Conversation
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 |
Will do :) |
Signed-off-by: Jonathan Burke <jonathan.burke@codecentric.de>
There was a problem hiding this 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]
Thanks and done 👍 |
Note: CI is blocked by #1189 or an alternative solution there (so don’t be alarmed by the test failure) |
Would make sense to have the new comment logic enabled when there is a specific flag (e.g., |
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 ^ |
Yeah, mine was more a temporary suggestion. |
PS. I just realised I posted this in the PR and not in the issue 😅 sorry |
This PR should address #1134 by sending all inline comments as a review combined.