Feature/f2018071301 add option for warnings per pr review #304

dimitrovv · 2018-07-13T08:25:13Z

Add option to limit comments per PR review
Create separate PR reviews depending on the comments to post

Fixes #302

Tries to resolve public_suffix to 3.0.2 but depends on Ruby >= 2.1

dimitrovv · 2018-07-13T09:00:17Z

@mmozuras @mknapik could someone review this PR, will appreciate it.

We need this, cause currently pronto is 💥 when trying to post many comments at once to GHE

dimitrovv · 2018-07-13T09:05:45Z

.travis.yml

@@ -3,18 +3,12 @@ before_install: gem update --system
 dist: trusty
 language: ruby
 rvm:
- 2.0.0


I have removed this, cause it tries to resolve public_suffix to 3.0.2, which requires Ruby version >= 2.1 🔴 build 403456633

@dimitrovv could you make a separate PR for Travis changes? And let's figure out that one first.

I saw you have already fixed this in 3b981f2

BurnazovA

Excellent solution!

mmozuras · 2018-07-15T10:17:07Z

.gitignore

@@ -4,3 +4,4 @@ pkg/*
 .DS_Store
 Gemfile.lock
 coverage
+.idea


@dimitrovv could you remove this change from this PR? Let's keep all strictly about warnings per PR review :)

mmozuras · 2018-07-15T10:17:47Z

.travis.yml

@@ -3,18 +3,12 @@ before_install: gem update --system
 dist: trusty
 language: ruby
 rvm:
- 2.0.0


@dimitrovv could you make a separate PR for Travis changes? And let's figure out that one first.

mmozuras · 2018-07-15T10:18:27Z

lib/pronto/config_file.rb

@@ -31,6 +31,7 @@ class ConfigFile
 'runners' => [],
 'formatters' => [],
 'max_warnings' => nil,
+ 'warnings_per_review' => nil,


@dimitrovv maybe we should have a high default? What do you think?

yes, it is possible, I was 🤔not to force this for all so someone can continue using the current flow.
Ex, if not supplied, it will be nil and the comments will not be separated into different PRs. However we've noticed Github is taking too much time and no review is posted if many comments are submitted at once, so we could change it to 30 I guess - would be enough, what do you think?

mmozuras · 2018-07-15T10:19:11Z

lib/pronto/github.rb

- }
- client.create_pull_request_review(slug, pull_id, options)
+ def publish_pull_request_comments(comments)
+ comments_left = comments.clone


@dimitrovv we prefer a style of comments_left_ = comments.clone instead of aligning it this way.

so I guess it would be better to leave it this way (without alignment) or:

comments_left = comments.clone warnings_per_review = @config.warnings_per_review || comments.size

Should I fix the alignment everywhere?

mmozuras · 2018-07-15T10:21:10Z

lib/pronto/formatter/github_pull_request_review_formatter.rb

@@ -10,7 +10,7 @@ def pretty_name
 end

 def submit_comments(client, comments)
- client.create_pull_request_review(comments)
+ client.publish_pull_request_comments(comments)


@dimitrovv why limit this feature to GitHub? Just trying to understand :)

yes, I thought about this, but I guess a review could only be submitted when using Github pronto client or I am wrong.

dimitrovv

@mmozuras thx for the review, will fix all things and will ping you again 👍

dimitrovv · 2018-07-15T12:23:50Z

lib/pronto/config_file.rb

@@ -31,6 +31,7 @@ class ConfigFile
 'runners' => [],
 'formatters' => [],
 'max_warnings' => nil,
+ 'warnings_per_review' => nil,


yes, it is possible, I was 🤔not to force this for all so someone can continue using the current flow.
Ex, if not supplied, it will be nil and the comments will not be separated into different PRs. However we've noticed Github is taking too much time and no review is posted if many comments are submitted at once, so we could change it to 30 I guess - would be enough, what do you think?

dimitrovv · 2018-07-15T12:29:11Z

lib/pronto/formatter/github_pull_request_review_formatter.rb

@@ -10,7 +10,7 @@ def pretty_name
 end

 def submit_comments(client, comments)
- client.create_pull_request_review(comments)
+ client.publish_pull_request_comments(comments)


yes, I thought about this, but I guess a review could only be submitted when using Github pronto client or I am wrong.

dimitrovv · 2018-07-15T12:31:22Z

lib/pronto/github.rb

- }
- client.create_pull_request_review(slug, pull_id, options)
+ def publish_pull_request_comments(comments)
+ comments_left = comments.clone


so I guess it would be better to leave it this way (without alignment) or:

comments_left = comments.clone warnings_per_review = @config.warnings_per_review || comments.size

dimitrovv · 2018-07-15T12:32:51Z

lib/pronto/github.rb

- }
- client.create_pull_request_review(slug, pull_id, options)
+ def publish_pull_request_comments(comments)
+ comments_left = comments.clone


Should I fix the alignment everywhere?

…_per_pr_review

Update Github Pronto client

Refactor specs a bit

dimitrovv · 2018-07-15T15:24:54Z

@mmozuras I've applied all changes you had requested. Could you review again so we can discuss the other items I've commented?

…_per_pr_review Resolve conflicts in github_spec.rb

dimitrovv · 2018-07-20T11:48:45Z

@mmozuras what do you think of this? Is it good and whether could be reviewed again / merged soon?

mmozuras · 2018-07-28T09:53:05Z

lib/pronto/config_file.rb

@@ -1,6 +1,7 @@
 module Pronto
 class ConfigFile
 DEFAULT_MESSAGE_FORMAT = '%{msg}'.freeze
+ DEFAULT_WARNINGS_PER_REVIEW = 30


@dimitrovv why is the default value 30? What's the GH rate limit?

@mmozuras we are using Github Enterprise and with disabled rate limit, but if I try to post about 40 comments per review GHE returns 502. I don't this is smth with the rate limit here, have not tried with Public Github. I suspect the server is not able to serve this request somehow.

Thus I can disable the default value here, so the comments will be separated into different reviews only if a default value is specified via config or env var. What do you think?

dimitrovv · 2018-08-24T16:15:36Z

@mmozuras what do you think, can we 🚢this soon, we need it when we have bigger PRs.

doomspork · 2019-05-14T20:43:49Z

@dimitrovv let's get this PR cleaned up so there aren't conflicts and I can give it a final once over and merge.

…_per_pr_review Resolve conflicts

dimitrovv · 2019-05-15T06:54:01Z

@doomspork, thanks, already done, please review when possible, we need this

doomspork · 2019-05-15T17:22:47Z

README.md

@@ -116,6 +116,16 @@ If you want review to appear on pull request diff, instead of comments:
 $ PRONTO_GITHUB_ACCESS_TOKEN=token pronto run -f github_pr_review -c origin/master
 ```

+All the comments will now not be published into a single PR review, but separated into a number of PRs.


@dimitrovv my only feedback is the wording here which is a little confusing to me. We don't mean to say it will create new PRs but will only leave N number of feedback per Pronto run, right?

If I create a PR with 100 errors, Pronto will report the first 30.
If I fix them, and push a new commit to my PR, Pronto will report the next 30.

Is that understanding correct? If so, I think we could rephrase this as such:

In order to avoid rate limiting by providers, Pronto publishes a limited number of comments per execution. By default, this is limited to the first 30 reported issues. You can change the number of reported issues either through the .pronto.yml file or the PRONTO_WARNINGS_PER_REVIEW environment variable.

@dimitrovv my only feedback is the wording here which is a little confusing to me.

Yes, I didn't define it the right way, will fix

If I create a PR with 100 errors, Pronto will report the first 30.
If I fix them, and push a new commit to my PR, Pronto will report the next 30.

Actually, it will post all N comments, but separated into X number of PR reviews (X = N / PRONTO_WARNINGS_PER_REVIEW). So all the comments should be published to Github only with a single pronto run

Thank you @dimitrovv

@doomspork, I've just tried to formulate the explanations around the new setting a bit better. Could you take a look when possible?

Fix the Readme for the warnings per review settings

doomspork · 2019-05-16T02:20:33Z

@prontolabs/core any final thoughts before I merge this?

…#304) * Add option for warnings per pr review * Use the warnings per pr review option to create consecutive pr reviews * Add .idea to gitignore * Fix specs * Fix previously failing specs * Remove extra empty line * Extend config specs * Fix and extend github specs * Bump the version to 0.9.6 * Change github client to work with a copy of comments * Extend github formatter specs * Restore version to 0.9.5 * Refactor github client spec * Add info about the new config option in README.md * Use old hash syntax in specs to satisfy ruby 2.1.0 * Remove ruby 2.0.0 from travis builds Tries to resolve public_suffix to 3.0.2 but depends on Ruby >= 2.1 * Restore .gitignore * Add default value of 30 to WARNING_PER_REVIEW Update Github Pronto client * Restore specs' default codding standards Refactor specs a bit * Update README.md * feature/f2018071301-add_option_for_warning_per_pr_review Fix the Readme for the warnings per review settings

chris-john-hopkins · 2021-11-01T17:43:41Z

Is it possible to disable the comments but fail the check? If I set the max_warnings to 0, the check passes even if there are rubocop errors.

dimitrovv added 16 commits July 13, 2018 00:55

Add option for warnings per pr review

4fab083

Use the warnings per pr review option to create consecutive pr reviews

486862c

Add .idea to gitignore

685f61e

Fix specs

48aa23e

Fix previously failing specs

8bb2cc4

Remove extra empty line

bec326d

Extend config specs

1d039da

Fix and extend github specs

60c683f

Bump the version to 0.9.6

3f76cb4

Change github client to work with a copy of comments

dfe7448

Extend github formatter specs

cca89f4

Restore version to 0.9.5

c546295

Refactor github client spec

2d36014

Add info about the new config option in README.md

8a6e25e

Use old hash syntax in specs to satisfy ruby 2.1.0

bf284c3

Remove ruby 2.0.0 from travis builds

a43ece1

Tries to resolve public_suffix to 3.0.2 but depends on Ruby >= 2.1

dimitrovv commented Jul 13, 2018

View reviewed changes

BurnazovA approved these changes Jul 13, 2018

View reviewed changes

mmozuras requested changes Jul 15, 2018

View reviewed changes

dimitrovv commented Jul 15, 2018

View reviewed changes

dimitrovv added 2 commits July 15, 2018 16:02

Merge branch 'master' into feature/f2018071301-add_option_for_warning…

1609117

…_per_pr_review

Restore .gitignore

c561421

emilpetkov approved these changes Jul 15, 2018

View reviewed changes

dimitrovv added 3 commits July 15, 2018 16:37

Add default value of 30 to WARNING_PER_REVIEW

90aacc8

Update Github Pronto client

Restore specs' default codding standards

9c4573b

Refactor specs a bit

Update README.md

f2913c7

Merge branch 'master' into feature/f2018071301-add_option_for_warning…

7df597e

…_per_pr_review Resolve conflicts in github_spec.rb

mmozuras reviewed Jul 28, 2018

View reviewed changes

drewish mentioned this pull request May 14, 2019

GitHub commenting with many issues triggers abuse filtering #133

Open

doomspork requested a review from a team May 14, 2019 20:43

Merge branch 'master' into feature/f2018071301-add_option_for_warning…

28a8b04

…_per_pr_review Resolve conflicts

doomspork reviewed May 15, 2019

View reviewed changes

doomspork requested review from a team and mmozuras May 15, 2019 17:23

feature/f2018071301-add_option_for_warning_per_pr_review

a622d31

Fix the Readme for the warnings per review settings

doomspork approved these changes May 16, 2019

View reviewed changes

mmozuras approved these changes May 18, 2019

View reviewed changes

doomspork merged commit 7eda9ca into prontolabs:master May 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/f2018071301 add option for warnings per pr review #304

Feature/f2018071301 add option for warnings per pr review #304

dimitrovv commented Jul 13, 2018 •

edited

Loading

dimitrovv commented Jul 13, 2018 •

edited

Loading

dimitrovv Jul 13, 2018

mmozuras Jul 15, 2018

dimitrovv Jul 15, 2018

BurnazovA left a comment

mmozuras Jul 15, 2018

mmozuras Jul 15, 2018

mmozuras Jul 15, 2018

dimitrovv Jul 15, 2018

mmozuras Jul 15, 2018

dimitrovv Jul 15, 2018

dimitrovv Jul 15, 2018

mmozuras Jul 15, 2018

dimitrovv Jul 15, 2018

mmozuras Jul 28, 2018

dimitrovv left a comment

dimitrovv Jul 15, 2018

dimitrovv Jul 15, 2018

dimitrovv Jul 15, 2018

dimitrovv Jul 15, 2018

dimitrovv commented Jul 15, 2018 •

edited

Loading

dimitrovv commented Jul 20, 2018

mmozuras Jul 28, 2018

dimitrovv Jul 28, 2018

dimitrovv commented Aug 24, 2018

doomspork commented May 14, 2019

dimitrovv commented May 15, 2019

doomspork May 15, 2019

dimitrovv May 15, 2019

doomspork May 15, 2019

dimitrovv May 15, 2019

doomspork commented May 16, 2019

chris-john-hopkins commented Nov 1, 2021

Feature/f2018071301 add option for warnings per pr review #304

Feature/f2018071301 add option for warnings per pr review #304

Conversation

dimitrovv commented Jul 13, 2018 • edited Loading

dimitrovv commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurnazovA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitrovv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitrovv commented Jul 15, 2018 • edited Loading

dimitrovv commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitrovv commented Aug 24, 2018

doomspork commented May 14, 2019

dimitrovv commented May 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doomspork commented May 16, 2019

chris-john-hopkins commented Nov 1, 2021

dimitrovv commented Jul 13, 2018 •

edited

Loading

dimitrovv commented Jul 13, 2018 •

edited

Loading

dimitrovv commented Jul 15, 2018 •

edited

Loading