Skip to content
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

feat: add pull review parsing and reward parsing #218

Open
wants to merge 68 commits into
base: development
Choose a base branch
from

Conversation

ishowvel
Copy link
Contributor

Resolves #60

@ishowvel ishowvel marked this pull request as draft December 14, 2024 15:49
@ishowvel ishowvel requested a review from 0x4007 December 20, 2024 15:00
@ishowvel ishowvel marked this pull request as ready for review December 20, 2024 15:00
@ishowvel
Copy link
Contributor Author

package.json Outdated Show resolved Hide resolved
Copy link

! No question provided

@gentlementlegen gentlementlegen changed the base branch from development to main January 22, 2025 03:31
@gentlementlegen gentlementlegen changed the base branch from main to development January 22, 2025 03:31
Copy link

! No question provided

@gentlementlegen
Copy link
Member

@shiv810 The command-ask seem to be running on each pull request comment now.

@shiv810
Copy link

shiv810 commented Jan 22, 2025

@shiv810 The command-ask seem to be running on each pull request comment now.

That's expected, assuming kernel is forwarding all pr review comments to command-ask.

https://github.com/ubiquity-os-marketplace/command-ask/blob/1595ad3f3fe262d0d8fb555351c0dc0e27cd9ecd/src/handlers/comment-created-callback.ts#L16-L23

It does exit as expected, given that, the command router does not give the question in the param. I'll take a look at command router.

Copy link

! No question provided

src/parser/review-incentivizer-module.ts Outdated Show resolved Hide resolved
src/parser/review-incentivizer-module.ts Outdated Show resolved Hide resolved
src/parser/github-comment-module.ts Outdated Show resolved Hide resolved
Copy link

! No question provided

2 similar comments
Copy link

! No question provided

Copy link

! No question provided

@whilefoo
Copy link
Member

@ishowvel let's finish this ASAP otherwise I will take over and finish

Copy link

! No question provided

@ishowvel
Copy link
Contributor Author

@whilefoo fixed cross fork diffs
heres the latest qa in which one of the pull request is across a fork
ishowvel-org/.ubiquity-os#74

@gentlementlegen
Copy link
Member

gentlementlegen commented Jan 23, 2025

Seems to work fine: Meniole#50 (comment)

With two pull-requests: Meniole#50 (comment)

I still think that this is dangerous to reward base on line count, since some files can be generated and because sometimes the pull-requests are not rebased and contain other pull-requests within it but only time will tell I suppose.

Copy link
Member

@whilefoo whilefoo left a comment

Choose a reason for hiding this comment

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

I tried running the plugin on ubiquity-os/plugins-wishlist#60 and found a bug where the plugin tries to list commits on PR ubiquity-os/plugins-wishlist/pull/218 instead of ubiquity-os-marketplace/text-conversation-rewards/pull/218 because it assumes that the PR is in the same repo as the issue but it's a wrong assumption.

Another similar bug is that .gitattributes is fetched from plugins-wishlist instead of text-conversation-rewards

You can get PR repo and owner with linkedPullReviews.self.base.repo.name and linkedPullReviews.self.base.repo.owner.login

After fixing these problems I was able to run the calculation and got this result:

 [ 1853.71 WXDAI ] 

@ishowvel
Contributions Overview
ViewContributionCountReward
IssueTask11600
ReviewCode Review7245.71
IssueComment18
Review Details for #218
ChangesPriorityReward
+3982 -4148181.3
+6210 -1990182
+22 -610.28
+92 -1311.05
+3779 -3729175.08
+319 -11514.34
+132 -3411.66
Conversation Incentives
CommentFormattingRelevancePriorityReward
The spec basically gives a reward to the diff of the commits com…
2
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 34
  wordValue: 0.1
  result: 2
148

 [ 884.18 WXDAI ] 

@0x4007
Contributions Overview
ViewContributionCountReward
ReviewCode Review2105.5
IssueSpecification1173.8
IssueComment8557.72
ReviewComment447.16
Review Details for #218
ChangesPriorityReward
+244 -1612.6
+7058 -32321102.9
Conversation Incentives
CommentFormattingRelevancePriorityReward
Reviews always seem to be slower than new pulls being submitted.…
43.45
content:
  content:
    p:
      score: 0
      elementCount: 26
    h1:
      score: 1
      elementCount: 1
    h2:
      score: 1
      elementCount: 4
    h3:
      score: 1
      elementCount: 5
    ul:
      score: 1
      elementCount: 8
    li:
      score: 0.5
      elementCount: 23
    ol:
      score: 1
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 35.5
regex:
  wordCount: 172
  wordValue: 0.1
  result: 7.95
14173.8
# Source Specification## uusd.ubq.fi#3Taking this large pull…
71.94
content:
  content:
    h1:
      score: 1
      elementCount: 1
    h2:
      score: 1
      elementCount: 1
    p:
      score: 0
      elementCount: 15
    h6:
      score: 1
      elementCount: 1
    h3:
      score: 1
      elementCount: 1
    ul:
      score: 1
      elementCount: 2
    li:
      score: 0.5
      elementCount: 8
    a:
      score: 5
      elementCount: 7
  result: 45
regex:
  wordCount: 320
  wordValue: 0.2
  result: 26.94
14287.76
A last consideration is the premium for priority level. Similar …
23.79
content:
  content:
    p:
      score: 0
      elementCount: 5
    h3:
      score: 1
      elementCount: 2
    a:
      score: 5
      elementCount: 2
  result: 12
regex:
  wordCount: 121
  wordValue: 0.2
  result: 11.79
1495.16
@kingsley-einstein the reason why it follows up is because you a…
3.29
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 27
  wordValue: 0.2
  result: 3.29
1413.16
This seems like it might be related to that one problem a long t…
9.08
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 89
  wordValue: 0.2
  result: 9.08
1436.32
I remember thinking of this in the past but I couldn't find my o…
4.5
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 39
  wordValue: 0.2
  result: 4.5
1418
That looks fine for now, we can keep iterating to find something…
4.01
content:
  content:
    p:
      score: 0
      elementCount: 1
    br:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 34
  wordValue: 0.2
  result: 4.01
1416.04
I think we should retain limits across the board for all incenti…
9.09
content:
  content:
    p:
      score: 0
      elementCount: 1
    ul:
      score: 1
      elementCount: 1
    li:
      score: 0.5
      elementCount: 3
  result: 2.5
regex:
  wordCount: 61
  wordValue: 0.2
  result: 6.59
1436.36
I think perhaps the conclusive review score should be based on X…
13.73
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 85
  wordValue: 0.2
  result: 8.73
1454.92
You should retain comments
0.32
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 4
  wordValue: 0.1
  result: 0.32
141.28
I imagine that there may be a different dedicated property name …
1.17
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 18
  wordValue: 0.1
  result: 1.17
144.68
The reviewers should get paid for every new line of code they re…
2.35
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 41
  wordValue: 0.1
  result: 2.35
149.4
![image](https://github.com/user-attachments/assets/2f3bb6b4-205…
7.95
content:
  content:
    h2:
      score: 1
      elementCount: 1
    img:
      score: 5
      elementCount: 1
    p:
      score: 0
      elementCount: 1
  result: 6
regex:
  wordCount: 33
  wordValue: 0.1
  result: 1.95
1431.8

 [ 8.52 WXDAI ] 

@Keyrxng
Contributions Overview
ViewContributionCountReward
IssueComment18.52
Conversation Incentives
CommentFormattingRelevancePriorityReward
I agree this would be great and solve a problem that's existed f…
8.52
content:
  content:
    p:
      score: 0
      elementCount: 2
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 66
  wordValue: 0.1
  result: 3.52
148.52

 [ 1.16 WXDAI ] 

@surafeldev
Contributions Overview
ViewContributionCountReward
IssueComment11.16
Conversation Incentives
CommentFormattingRelevancePriorityReward
I have been researching and find out how should it be done, I ha…
1.17
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 18
  wordValue: 0.1
  result: 1.17
141.16

 [ 506.5 WXDAI ] 

@gentlementlegen
Contributions Overview
ViewContributionCountReward
ReviewBase Review for #218125
ReviewCode Review8306.86
IssueComment124
ReviewComment18150.64
Review Details for #218
ChangesPriorityReward
+3963 -4148181.11
+19 -410.23
+6232 -1992182.24
+92 -1311.05
+4070 -3816178.86
+132 -3411.66
+0 -010
+2757 -3414161.71
Conversation Incentives
CommentFormattingRelevancePriorityReward
I think [this pull-request](https://github.com/ubiquity-os-marke…
6
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 15
  wordValue: 0.1
  result: 1
1424
Don't we want some defaults here? Plus a description and an exam…
0.88
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 13
  wordValue: 0.1
  result: 0.88
143.52
add `{ }` to the else block
0.39
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 5
  wordValue: 0.1
  result: 0.39
141.56
```suggestion```
0
content:
  content: {}
  result: 0
regex:
  wordCount: 0
  wordValue: 0.1
  result: 0
140
There can be multiple assignees to an issue, is this case handle…
0.83
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 12
  wordValue: 0.1
  result: 0.83
143.32
I believe this is meant to pick up a single PR, but are you sure…
1.7
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 28
  wordValue: 0.1
  result: 1.7
146.8
Why is the reward here `1200`? It should be `800`…
0.71
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 10
  wordValue: 0.1
  result: 0.71
142.84
@ishowvel I can do `/start @ishowvel` which would assign…
1.22
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 19
  wordValue: 0.1
  result: 1.22
144.88
@whilefoo wouldn't there be only one that would technically clos…
9.23
content:
  content:
    p:
      score: 0
      elementCount: 3
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 82
  wordValue: 0.1
  result: 4.23
1436.92
Given that the plugin does not generate and re-opens the issue i…
1.75
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 29
  wordValue: 0.1
  result: 1.75
147
Why is the `* 3` hard coded here?
0.46
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 6
  wordValue: 0.1
  result: 0.46
141.84
```suggestionreviewIncentivizer: {}```…
0.65
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 9
  wordValue: 0.1
  result: 0.65
142.6
Yes, but it doesn't mean to hard code a times 3 in the code. As …
3.38
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 63
  wordValue: 0.1
  result: 3.38
1413.52
Please bump to `2.0.2`
0.25
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 3
  wordValue: 0.1
  result: 0.25
141
Shouldn't this be 1200, or is it a reason why it is back to 800
1.06
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 16
  wordValue: 0.1
  result: 1.06
144.24
You should add the module configuration to the readme, and insid…
5.77
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 11
  wordValue: 0.1
  result: 0.77
1423.08
@ishowvel Please redo a QA with multiple pull-requests and assgi…
0.77
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 11
  wordValue: 0.1
  result: 0.77
143.08
@shiv810 The `command-ask` seem to be running on each pu…
0.83
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 12
  wordValue: 0.1
  result: 0.83
143.32
Seems to work fine: https://github.com/Meniole/text-conversation…
7.78
content:
  content:
    p:
      score: 0
      elementCount: 3
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 50
  wordValue: 0.1
  result: 2.78
1431.12

 [ 526.15 WXDAI ] 

@whilefoo
Contributions Overview
ViewContributionCountReward
ReviewBase Review for #218125
ReviewCode Review8245.63
IssueComment116.04
ReviewComment33239.48
Review Details for #218
ChangesPriorityReward
+3982 -4148181.3
+6210 -1990182
+22 -610.28
+92 -1311.05
+3927 -3767176.94
+14 -2410.38
+110 -1911.29
+175 -6412.39
Conversation Incentives
CommentFormattingRelevancePriorityReward
Maybe this can be done after this task but I think we should als…
4.01
content:
  content:
    p:
      score: 0
      elementCount: 2
  result: 0
regex:
  wordCount: 77
  wordValue: 0.1
  result: 4.01
1416.04
![image](https://github.com/user-attachments/assets/daa6bc37-095…
6.9
content:
  content:
    p:
      score: 0
      elementCount: 2
    img:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 32
  wordValue: 0.1
  result: 1.9
1427.6
you need to check array length or use `?.`
0.59
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 8
  wordValue: 0.1
  result: 0.59
142.36
conclusive review means either `APPROVED` or `CHANGE…
0.39
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 5
  wordValue: 0.1
  result: 0.39
141.56
you can use `Object.entries`
0.25
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 3
  wordValue: 0.1
  result: 0.25
141
you should exclude files in `.gitattributes` marked as &…
5.59
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 8
  wordValue: 0.1
  result: 0.59
1422.36
```suggestionfor (const username of Object.key…
0
content:
  content: {}
  result: 0
regex:
  wordCount: 0
  wordValue: 0.1
  result: 0
140
```suggestionconst reward = result[key];&#…
0
content:
  content: {}
  result: 0
regex:
  wordCount: 0
  wordValue: 0.1
  result: 0
140
This doesn't seem correct. you are comparing commits between the…
3.7
content:
  content:
    p:
      score: 0
      elementCount: 2
  result: 0
regex:
  wordCount: 70
  wordValue: 0.1
  result: 3.7
1414.8
why is base reward 0 here?
0.46
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 6
  wordValue: 0.1
  result: 0.46
141.84
why did this change?
0.32
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 4
  wordValue: 0.1
  result: 0.32
141.28
because this runs in a loop it seems inefficient to read files e…
0.94
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 14
  wordValue: 0.1
  result: 0.94
143.76
I think you need the commit before the first one, because when y…
3.43
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 64
  wordValue: 0.1
  result: 3.43
1413.72
here you are also reading files instead of fetching from Github
0.77
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 11
  wordValue: 0.1
  result: 0.77
143.08
```suggestionconst firstCommitSha = pullCommit…
0.46
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 6
  wordValue: 0.1
  result: 0.46
141.84
`excludedFilePatterns.every((pattern) => minimatch(fileNa…
1.8
content:
  content:
    p:
      score: 0
      elementCount: 2
  result: 0
regex:
  wordCount: 30
  wordValue: 0.1
  result: 1.8
147.2
I'm not certain but in Github API example it shows that both …
2.15
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 37
  wordValue: 0.1
  result: 2.15
148.6
you should check for undefined and throw error
0.59
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 8
  wordValue: 0.1
  result: 0.59
142.36
use logger
0.18
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 2
  wordValue: 0.1
  result: 0.18
140.72
why return if there's an assignee?
0.52
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 7
  wordValue: 0.1
  result: 0.52
142.08
What if there are multiple assignees?
0.46
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 6
  wordValue: 0.1
  result: 0.46
141.84
I realized that `data.linkedReviews` already has reviews…
15.39
content:
  content:
    p:
      score: 0
      elementCount: 3
    a:
      score: 5
      elementCount: 2
  result: 10
regex:
  wordCount: 109
  wordValue: 0.1
  result: 5.39
1461.56
I haven't seen it happen but theoretically if an issue has 2 ass…
1.75
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 29
  wordValue: 0.1
  result: 1.75
147
@ishowvel then please use reviews from `data.linkedReviews&#…
6.65
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 27
  wordValue: 0.1
  result: 1.65
1426.6
it would be easier to read this if it was inverted - `.every…
0.77
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 11
  wordValue: 0.1
  result: 0.77
143.08
I'm guessing you accidentally committed this?
0.52
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 7
  wordValue: 0.1
  result: 0.52
142.08
this is equivalent to `.find()`
0.32
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 4
  wordValue: 0.1
  result: 0.32
141.28
Why do you still fetch linked pulls if you have them in `dat…
0.83
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 12
  wordValue: 0.1
  result: 0.83
143.32
```suggestionfor (const username of Object.k…
0
content:
  content: {}
  result: 0
regex:
  wordCount: 0
  wordValue: 0.1
  result: 0
140
still here
0.18
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 2
  wordValue: 0.1
  result: 0.18
140.72
I'm not sure if this will work if the PR head branch is from the…
1.11
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 17
  wordValue: 0.1
  result: 1.11
144.44
Sometimes what happens is that a reviewer submits multiple revie…
1.65
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 27
  wordValue: 0.1
  result: 1.65
146.6
Display pull request number
0.32
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 4
  wordValue: 0.1
  result: 0.32
141.28
@ishowvel let's finish this ASAP otherwise I will take over and …
0.88
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 13
  wordValue: 0.1
  result: 0.88
143.52

 [ 7.2 WXDAI ] 

@shiv810
Contributions Overview
ViewContributionCountReward
ReviewComment17.2
Conversation Incentives
CommentFormattingRelevancePriorityReward
That's expected, assuming kernel is forwarding all pr review com…
7.2
content:
  content:
    p:
      score: 0
      elementCount: 3
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 38
  wordValue: 0.1
  result: 2.2
147.2

The odd thing is that @ishowvel got 7 code reviews even though they are the author and don't have any reviews

@ishowvel
Copy link
Contributor Author

ishowvel commented Jan 23, 2025

@whilefoo this seems to not be an issue with the code, i manually fetched reviews and saved it to a file while debugging

await getPullRequestReviews(this.context, {
        owner: "ubiquity-os-marketplace",
        repo: "text-conversation-rewards",
        pull_number: 218,
      })

reviews.json

the issue doesnt seem to be with the code but the fact that github is returning these reviews, for now i can add a check for reviews being skipped for pull author

@ishowvel ishowvel requested a review from whilefoo January 23, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reviewer Incentives
5 participants