-
Notifications
You must be signed in to change notification settings - Fork 245
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
Feature/f2018071301 add option for warnings per pr review #304
Feature/f2018071301 add option for warnings per pr review #304
Conversation
Tries to resolve public_suffix to 3.0.2 but depends on Ruby >= 2.1
.travis.yml
Outdated
@@ -3,18 +3,12 @@ before_install: gem update --system | |||
dist: trusty | |||
language: ruby | |||
rvm: | |||
- 2.0.0 |
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.
I have removed this, cause it tries to resolve public_suffix to 3.0.2, which requires Ruby version >= 2.1 🔴 build 403456633
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.
@dimitrovv could you make a separate PR for Travis changes? And let's figure out that one first.
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.
I saw you have already fixed this in 3b981f2
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.
Excellent solution!
.gitignore
Outdated
@@ -4,3 +4,4 @@ pkg/* | |||
.DS_Store | |||
Gemfile.lock | |||
coverage | |||
.idea |
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.
@dimitrovv could you remove this change from this PR? Let's keep all strictly about warnings per PR review :)
.travis.yml
Outdated
@@ -3,18 +3,12 @@ before_install: gem update --system | |||
dist: trusty | |||
language: ruby | |||
rvm: | |||
- 2.0.0 |
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.
@dimitrovv could you make a separate PR for Travis changes? And let's figure out that one first.
lib/pronto/config_file.rb
Outdated
@@ -31,6 +31,7 @@ class ConfigFile | |||
'runners' => [], | |||
'formatters' => [], | |||
'max_warnings' => nil, | |||
'warnings_per_review' => nil, |
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.
@dimitrovv maybe we should have a high default? What do you think?
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.
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?
lib/pronto/github.rb
Outdated
} | ||
client.create_pull_request_review(slug, pull_id, options) | ||
def publish_pull_request_comments(comments) | ||
comments_left = comments.clone |
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.
@dimitrovv we prefer a style of comments_left_ = comments.clone
instead of aligning it this way.
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.
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
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.
Should I fix the alignment everywhere?
@@ -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) |
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.
@dimitrovv why limit this feature to GitHub? Just trying to understand :)
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.
yes, I thought about this, but I guess a review could only be submitted when using Github pronto client or I am wrong.
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.
👍
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.
@mmozuras thx for the review, will fix all things and will ping you again 👍
lib/pronto/config_file.rb
Outdated
@@ -31,6 +31,7 @@ class ConfigFile | |||
'runners' => [], | |||
'formatters' => [], | |||
'max_warnings' => nil, | |||
'warnings_per_review' => nil, |
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.
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?
@@ -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) |
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.
yes, I thought about this, but I guess a review could only be submitted when using Github pronto client or I am wrong.
lib/pronto/github.rb
Outdated
} | ||
client.create_pull_request_review(slug, pull_id, options) | ||
def publish_pull_request_comments(comments) | ||
comments_left = comments.clone |
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.
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
lib/pronto/github.rb
Outdated
} | ||
client.create_pull_request_review(slug, pull_id, options) | ||
def publish_pull_request_comments(comments) | ||
comments_left = comments.clone |
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.
Should I fix the alignment everywhere?
Update Github Pronto client
Refactor specs a bit
@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
@mmozuras what do you think of this? Is it good and whether could be reviewed again / merged soon? |
@@ -1,6 +1,7 @@ | |||
module Pronto | |||
class ConfigFile | |||
DEFAULT_MESSAGE_FORMAT = '%{msg}'.freeze | |||
DEFAULT_WARNINGS_PER_REVIEW = 30 |
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.
@dimitrovv why is the default value 30? What's the GH rate limit?
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.
@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?
@mmozuras what do you think, can we 🚢this soon, we need it when we have bigger PRs. |
@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
@doomspork, thanks, already done, please review when possible, we need this |
README.md
Outdated
@@ -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. |
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.
@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 thePRONTO_WARNINGS_PER_REVIEW
environment variable.
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.
@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
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.
Thank you @dimitrovv
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.
@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
@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
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. |
Fixes #302