Skip to content
This repository has been archived by the owner on Jun 12, 2024. It is now read-only.

Rank effort to review a given PR #109

Merged
merged 12 commits into from
Feb 2, 2023
Merged

Rank effort to review a given PR #109

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2367326
wip guess the effort of a review
Jan 25, 2023
5a25439
make a test
Jan 25, 2023
6fd9f02
basic tests to workout scale
Jan 25, 2023
55463d0
make sure new are regular
Jan 25, 2023
c01b9a8
update more unit tests
Jan 27, 2023
e7d6760
Merge branch 'main' into rank-effort
prince-chrismc Jan 30, 2023
6e60b91
make sure this is a regular
prince-chrismc Jan 30, 2023
aafec52
less strict for regular number of files changed
prince-chrismc Jan 30, 2023
411a2dc
Apply suggestions from code review
prince-chrismc Feb 1, 2023
e70a1c8
Update internal/format/reviews.go
prince-chrismc Feb 1, 2023
9b76595
Fix tests
prince-chrismc Feb 1, 2023
f12e586
Update pending_review/pull_request.go
prince-chrismc Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/format/merge_ready.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,16 @@ func ReadyToMerge(prs []*pending_review.PullRequestSummary) string {
return ""
}

breif := "**1** pull request is"
brief := "**1** pull request is"
if rowCount > 1 {
breif = "**" + fmt.Sprint(rowCount) + "** pull requests are"
brief = "**" + fmt.Sprint(rowCount) + "** pull requests are"
}

return `

### :heavy_check_mark: Ready to Merge

Currently ` + breif + ` waiting to be merged :tada:
Currently ` + brief + ` waiting to be merged :tada:


PR | By | Opened | Recipe | Reviews | :star2: Approvers
Expand Down
20 changes: 19 additions & 1 deletion internal/format/reviews.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func underWay(pr *pending_review.PullRequestSummary) string {
var retval string
title := title(pr.Change, pr.Recipe)
if !pr.CciBotPassed {
// The assumption here is that "no passing" means in progress since "failing" PRs are disgarded
// The assumption here is that "no passing" means in progress since "failing" PRs are discarded
title = ":stopwatch: " + pr.Recipe
}

Expand All @@ -41,6 +41,7 @@ func underWay(pr *pending_review.PullRequestSummary) string {
fmt.Sprint("[", pr.OpenedBy, "](https://github.com/", pr.OpenedBy, ")"),
pr.CreatedAt.Format("Jan 2"),
title,
weight(pr.Weight),
fmt.Sprint(pr.Summary.Count),
lastReviewTime(pr),
strings.Join(pr.Summary.Blockers, ", "),
Expand Down Expand Up @@ -92,6 +93,23 @@ func title(change pending_review.Category, recipe string) string {
return "???"
}

func weight(w pending_review.ReviewWeight) string {
switch w {
case pending_review.TINY:
return "tiny"
case pending_review.SMALL:
return "small"
case pending_review.REGULAR:
return "regular"
case pending_review.HEAVY:
return ":warning: heavy"
case pending_review.TOO_MUCH:
return ":stop_sign: too much"
prince-chrismc marked this conversation as resolved.
Show resolved Hide resolved
}

return "???"
}

func lastReviewTime(pr *pending_review.PullRequestSummary) string {
if pr.Summary.LastReview != nil {
date := pr.Summary.LastReview.SubmittedAt.Format("Jan 2")
Expand Down
8 changes: 5 additions & 3 deletions internal/format/reviews_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestFormatMarkdownRows(t *testing.T) {

reviewRow, reviewCount := ReviewsToMarkdownRows(rs, false)
assert.Equal(t, reviewCount, 1)
assert.Equal(t, "[#4556](https://github.com/conan-io/conan-center-index/pull/4556)|[anton-danielsson](https://github.com/anton-danielsson)|Jan 1|:memo: protobuf|36|Apr 9 :bell:|uilianries|prince-chrismc\n", reviewRow)
assert.Equal(t, "[#4556](https://github.com/conan-io/conan-center-index/pull/4556)|[anton-danielsson](https://github.com/anton-danielsson)|Jan 1|:memo: protobuf|tiny|36|Apr 9 :bell:|uilianries|prince-chrismc\n", reviewRow)
prince-chrismc marked this conversation as resolved.
Show resolved Hide resolved
}

func TestFormatMarkdownRowsDocs(t *testing.T) {
Expand Down Expand Up @@ -125,6 +125,7 @@ func TestFormatMarkdownRowsCiPending(t *testing.T) {
"CreatedAt": "2021-12-28T02:27:59Z",
"Recipe": "libkmod",
"Change": 0,
"Weight": 6,
"ReviewURL": "https://github.com/conan-io/conan-center-index/pull/8557",
"LastCommitSHA": "88e25c1a89cbc4b130e37fb3d42fe7e16cf3b4ca",
"LastCommitAt": "2021-12-28T02:26:17Z",
Expand All @@ -144,7 +145,7 @@ func TestFormatMarkdownRowsCiPending(t *testing.T) {

mergeRow, mergeCount := ReviewsToMarkdownRows(rs, false)
assert.Equal(t, mergeCount, 1)
assert.Equal(t, "[#8557](https://github.com/conan-io/conan-center-index/pull/8557)|[daravi](https://github.com/daravi)|Dec 28|:stopwatch: libkmod|0|:eyes:||\n", mergeRow)
assert.Equal(t, "[#8557](https://github.com/conan-io/conan-center-index/pull/8557)|[daravi](https://github.com/daravi)|Dec 28|:stopwatch: libkmod|???|0|:eyes:||\n", mergeRow)
}

func TestFormatMarkdownRowsCiSuccess(t *testing.T) {
Expand All @@ -156,6 +157,7 @@ func TestFormatMarkdownRowsCiSuccess(t *testing.T) {
"CreatedAt": "2021-12-28T02:27:59Z",
"Recipe": "libkmod",
"Change": 0,
"Weight": 2,
"ReviewURL": "https://github.com/conan-io/conan-center-index/pull/8557",
"LastCommitSHA": "88e25c1a89cbc4b130e37fb3d42fe7e16cf3b4ca",
"LastCommitAt": "2021-12-28T02:26:17Z",
Expand All @@ -175,5 +177,5 @@ func TestFormatMarkdownRowsCiSuccess(t *testing.T) {

mergeRow, mergeCount := ReviewsToMarkdownRows(rs, false)
assert.Equal(t, mergeCount, 1)
assert.Equal(t, "[#8557](https://github.com/conan-io/conan-center-index/pull/8557)|[daravi](https://github.com/daravi)|Dec 28|:new: libkmod|0|:eyes:||\n", mergeRow)
assert.Equal(t, "[#8557](https://github.com/conan-io/conan-center-index/pull/8557)|[daravi](https://github.com/daravi)|Dec 28|:new: libkmod|regular|0|:eyes:||\n", mergeRow)
prince-chrismc marked this conversation as resolved.
Show resolved Hide resolved
}
10 changes: 5 additions & 5 deletions internal/format/under_way.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@ func UnderReview(prs []*pending_review.PullRequestSummary, owner string, repo st
`
}

breif := "is **1** pull request"
brief := "is **1** pull request"
if rowCount > 1 {
breif = "are **" + fmt.Sprint(rowCount) + "** pull requests"
brief = "are **" + fmt.Sprint(rowCount) + "** pull requests"
}

return `

### :nerd_face: Please Review!

There ` + breif + ` currently under way :detective:
There ` + brief + ` currently under way :detective:

PR | By | Opened | Recipe | Reviews | Last | :stop_sign: Blockers | :star2: Approvers
:---: | --- | --- | --- | :---: | --- | --- | ---
PR | By | Opened | Recipe | Weight | Reviews | Last | :stop_sign: Blockers | :star2: Approvers
:---: | --- | --- | --- | --- | :---: | --- | --- | ---
` +
tableBody
}
62 changes: 54 additions & 8 deletions pending_review/pull_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ const (
CONFIG Category = iota
)

// ReviewWeight attempts to qualify the amount of effort to review any given pull request
type ReviewWeight int

// ReviewWeight attempts to qualify the amount of effort to review any given pull request
const (
TINY ReviewWeight = iota
SMALL ReviewWeight = iota
REGULAR ReviewWeight = iota
HEAVY ReviewWeight = iota
TOO_MUCH ReviewWeight = iota
)

// PullRequestSummary regarding its location in the review process of conan-center-index.
// See https://github.com/conan-io/conan-center-index/blob/master/docs/review_process.md
// for more information
Expand All @@ -28,6 +40,7 @@ type PullRequestSummary struct {
CreatedAt time.Time
Recipe string
Change Category
Weight ReviewWeight
ReviewURL string
LastCommitSHA string
LastCommitAt time.Time
Expand Down Expand Up @@ -90,13 +103,14 @@ func (s *PullRequestService) GetReviewSummary(ctx context.Context, owner string,
return nil, nil, err
}

diff, resp, err := s.determineTypeOfChange(ctx, owner, repo, p.Number, 10 /* recipes are currently 5-7 files */)
diff, resp, err := s.determineTypeOfChange(ctx, owner, repo, p.Number, 14 /* recipes are currently 8-10 files */)
if err != nil {
return nil, resp, err
}

p.Recipe = diff.Recipe
p.Change = diff.Change
p.Weight = diff.Weight

reviews, resp, err := s.ListAllReviews(ctx, owner, repo, p.Number)
if err != nil {
Expand Down Expand Up @@ -161,6 +175,7 @@ func processLabels(labels []*Label) error {
type change struct {
Recipe string
Change Category
Weight ReviewWeight
}

func (s *PullRequestService) determineTypeOfChange(ctx context.Context, owner string, repo string, number int, perPage int) (*change, *Response, error) {
Expand All @@ -176,27 +191,58 @@ func (s *PullRequestService) determineTypeOfChange(ctx context.Context, owner st
return nil, resp, fmt.Errorf("%w", ErrInvalidChange)
}

change, err := getDiff(files[0])
change, err := processChangedFiles(files)
if err != nil {
return nil, resp, err
}

return change, resp, nil
}

func processChangedFiles(files []*CommitFile) (*change, error) {
if len(files) < 1 {
return nil, fmt.Errorf("%w", ErrInvalidChange)
}

change, err := getDiff(files[0])
if err != nil {
return nil, err
}

addition := files[0].GetAdditions()
deletions := files[0].GetDeletions()
for _, file := range files[1:] {
obtained, err := getDiff(file)
if err != nil {
return nil, resp, err
return nil, err
}

if change.Recipe != obtained.Recipe { // PR should only be changing one recipe at a time
return nil, resp, fmt.Errorf("%w", ErrInvalidChange)
if change.Recipe != obtained.Recipe {
return nil, fmt.Errorf("%w", ErrInvalidChange)
}

if change.Change == NEW && obtained.Change == EDIT {
change.Change = EDIT // Any edit breaks the "new recipe" definition
change.Change = EDIT
}

addition += file.GetAdditions()
deletions += file.GetDeletions()
}

return change, resp, nil
//if len(files) <= 2 && addition <= 10 && deletions == 0 {
if len(files) <= 2 && (addition+deletions) <= 10 {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love some feedback 🙏

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The numbers seem ok for me. I worry that it will not be until we actually try these numbers that we'll know if they are balanced properly, but my gut says yes :)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There a nice example, #109 (comment)

Reasonable is all I need on a first pass ;) I picked a few PRs while reviewing and said that seems about right 😆

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As first step I think is okay. We can update in case you feel different in the future.

change.Weight = TINY
} else if len(files) <= 4 && (addition+deletions) <= 25 {
change.Weight = SMALL
} else if len(files) <= 5 && (addition+deletions) <= 100 {
change.Weight = REGULAR
} else if len(files) <= 9 && addition <= 225 && deletions == 0 { // Basic new recipe addition with `test_v1_package`
change.Weight = REGULAR
ericLemanissier marked this conversation as resolved.
Show resolved Hide resolved
} else if len(files) > 12 || (addition+deletions) >= 500 {
change.Weight = TOO_MUCH
}
prince-chrismc marked this conversation as resolved.
Show resolved Hide resolved

return change, nil
}

func getDiff(file *CommitFile) (*change, error) {
Expand Down Expand Up @@ -232,5 +278,5 @@ func getDiff(file *CommitFile) (*change, error) {
return nil, fmt.Errorf("%w", ErrInvalidChange)
}

return &change{title, status}, nil
return &change{title, status, HEAVY}, nil // Default to heavy to make the calculation easier in `processChangedFiles`
}
Loading