-
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
#188: implement calculate average hoc and number of files changed in recent merged PRs #285
Conversation
@Suban05 can you review it, please? (Test fail, because this zerocracy/fbe#70 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 It looks good, but I have a small question
f.average_pull_hoc_size = hocs.empty? ? 0 : hocs.inject(&:+).to_f / hocs.size | ||
f.average_pull_files_size = files.empty? ? 0 : files.inject(&:+).to_f / files.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.
@Yegorov can we write it in a simpler way?
like this:
f.average_pull_hoc_size = hocs.empty? ? 0 : hocs.inject(&:+).to_f / hocs.size | |
f.average_pull_files_size = files.empty? ? 0 : files.inject(&:+).to_f / files.size | |
f.average_pull_hoc_size = hocs.sum / hocs.size.to_f | |
f.average_pull_files_size = files.sum / files.size.to_f |
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 I think not, because if hocs is empty, we will get an answer NaN
3.3.4 :002 > 0 / 0.to_f
=> NaN
The same thing for files
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 yes, indeed, you're right. what do you think about replacing hocs.inject(&:+)
with hocs.sum
?
like this:
hocs.empty? ? 0 : hocs.sum / hocs.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.
@Suban05 Yes, it has become more readable, thanks!
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 looks good
…anged in recent merged PRs
@Yegorov what's up with this one? Good to merge? Please, read this: https://www.yegor256.com/2024/04/01/ping-me-please.html |
@yegor256 yes of course, merge this PR. |
@Yegorov thanks! |
@Suban05 Thank you for your code review! We appreciate your contribution. As per our policy, you've earned +5 points: +10 base points, -10 for having fewer than 6 comments (only 4 were made), and +5 to meet the minimum reward. While we encourage more detailed reviews in the future, your effort is valued. Your current balance stands at +443. Keep up the good work! |
@Yegorov Hey there, awesome job on your contribution! 🎉 You've scored a cool 30 points: 15 for showing up, and another 15 for those 159 hits-of-code you cranked out. That's a sweet spot - not too much to trigger deductions, but enough to max out your code bonus. Keep the momentum going and aim for that perfect balance between quantity and quality. Your total now stands at an impressive +401. Rock on and keep those contributions flowing! |
@Suban05 ok thanks for the advice! |
@yegor256 Merge zerocracy/fbe#70 PR before