-
Notifications
You must be signed in to change notification settings - Fork 5
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
#338: fix review times in QoS #342
Conversation
@Suban05 can you review, please. Test fail, because this zerocracy/fbe#99 PR need merge before |
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.
@Yegorov all good, but I have a small question
@@ -155,8 +155,8 @@ | |||
"repo:#{repo} type:pr is:merged closed:>#{f.since.utc.iso8601[0..9]}" | |||
)[:items].each do |pr| | |||
pr_reviews = Fbe.octo.pull_request_reviews(repo, pr[:number]) | |||
pr_review = pr_reviews.min_by { |r| r[:submitted_at] } | |||
review_times << (pr[:merged_at] - pr_review[:submitted_at]).to_i if pr_review | |||
pr_review = pr_reviews.reject { |r| r[:submitted_at].nil? }.min_by { |r| r[:submitted_at] } |
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.
@Yegorov what do you think about the using filter_map
?
pr_reviews.filter_map { |r| r if r[:submitted_at] }
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.
@Suban05 if use filter_map
i get Rubocop warning
judges/quality-of-service/quality-of-service.rb:158:30: C: [Correctable] Style/MapCompactWithConditionalBlock: Replace filter_map { ... } with select.
pr_review = pr_reviews.filter_map { |r| r if r[:submitted_at] }.min_by { |r| r[:submitted_at] }
As an option, may be use select
? What do you think?
pr_review = pr_reviews.select { |r| r[:submitted_at] }.min_by { |r| r[:submitted_at] }
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.
@Yegorov good idea, let's use select
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.
@Suban05 thanks, fixed
@yegor256 please check |
@Yegorov thanks! |
@Yegorov Thanks for your contribution! We appreciate your effort, but there's room for improvement. You've earned +4 points, which is the minimum reward according to our policy. To maximize your bonus, consider adding more hits-of-code, ensuring code reviews, and encouraging reviewer comments. Keep up the good work and aim higher next time! Your running balance is +194. |
@Yegorov Thank you for your contribution! We've reviewed your work and awarded you +4 points, which aligns with our team's policy for code contributions. While we appreciate your effort, please note that we encourage more substantial contributions and thorough code reviews. Your running balance is now +194. We look forward to seeing more of your work and remember to focus on both speed and quality in your future contributions. |
Closes #338