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.

Sign up for GitHub

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

Jump to bottom

Pull Request State Adjustments #59

Closed
0x4007 opened this issue Dec 4, 2024 · 16 comments · Fixed by #61
Closed

Pull Request State Adjustments #59

0x4007 opened this issue Dec 4, 2024 · 16 comments · Fixed by #61
Assignees
@gentlementlegen
Labels
Price: 75 USD Priority: 3 (High) Time: <1 Hour

Comments

@0x4007
Copy link
Member

0x4007 commented Dec 4, 2024 •
edited by ubiquity-os-beta bot
Loading

  • When a follow up occurs, the pull request should be turned into a draft.
  • If it is already a draft, only leave the follow up message.1
  • When a disqualification occurs, the pull request should be closed.

Context: ubiquity-os/plugins-wishlist#60 (comment)

Rationale: when a contributor converts from draft to ready, then that signals it is ready for review.

Footnotes

  1. ⚠ 52% possible duplicate - Reviewer Follow Ups ↩
@ubiquity-os-beta ubiquity-os-beta
Copy link

Note

The following contributors may be suitable for this task:

zugdev

1% Match ubiquity/ubq.fi#40

gentlementlegen

1% Match ubiquity-os/ubiquity-os-kernel#125

1 similar comment
@ubiquity-os-beta ubiquity-os-beta
Copy link

Note

The following contributors may be suitable for this task:

zugdev

1% Match ubiquity/ubq.fi#40

gentlementlegen

1% Match ubiquity-os/ubiquity-os-kernel#125

@0x4007 0x4007 mentioned this issue Dec 4, 2024
Open
Closed
@gentlementlegen
Copy link
Member

Just to make sure, when the PR is converted back to a draft it still sends a reminder message correct?

The closing of the pull-request is currently handled by https://github.com/ubiquity-os-marketplace/command-start-stop.

@gentlementlegen
Copy link
Member

/start

@ubiquity-os-beta ubiquity-os-beta
Copy link

Deadline Mon, Dec 9, 11:03 AM UTC
Beneficiary 0x0fC1b909ba9265A846b82CF4CE352fc3e7EeB2ED

Tip

  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@0x4007
Copy link
Member Author

0x4007 commented Dec 9, 2024

Just to make sure, when the PR is converted back to a draft it still sends a reminder message correct?

Yes

The closing of the pull-request is currently handled by https://github.com/ubiquity-os-marketplace/command-start-stop.

When disqualified it needs to close the pull

@gentlementlegen gentlementlegen mentioned this issue Dec 9, 2024
Merged
@whilefoo
Copy link
Contributor

What if the user is waiting for a PR review? in that case it doesn't make sense to turn it into draft

@0x4007
Copy link
Member Author

0x4007 commented Dec 10, 2024 •
edited
Loading

What if the user is waiting for a PR review? in that case it doesn't make sense to turn it into draft

Let me try and think through the scenarios, ✅ means "success condition" or the assignee is not at fault and should not be penalized:

  • Assignee just opened a pull and nobody reviewed yet. ✅1
  • Assignee received an approval. ✅
  • Assignee received a rejection. ❌
  • Assignee received a review comment. ❌
  • Assignee stopped commenting or committing, excluding one of the previously defined success conditions. ❌

Footnotes

  1. The follow ups are handy for reviewers to be nudged in their notifications. They work even as is: reminding the assignee; but perhaps it makes more sense to nudge the issue author in the pull. I suppose this should effectively pause the timer while nudging the collaborators. If the issue author is not a collaborator, or is the assignee, then we have a problem determining which collaborator to nudge. ↩

@gentlementlegen
Copy link
Member

I think it should be only for explicit rejection (as in "requested changes"). There are plenty of times we comment for a clarification or simple questions, which are not negative / requesting changes.

@0x4007
Copy link
Member Author

0x4007 commented Dec 11, 2024

I think it should be only for explicit rejection (as in "requested changes"). There are plenty of times we comment for a clarification or simple questions, which are not negative / requesting changes.

It's not accepted so there is work they need to do, even if it means to answer the question.

@gentlementlegen
Copy link
Member

@0x4007 Correct but the need of answering a question should not turn the pull-request into a draft no? Because the pull-request code itself won't change, that was my reasoning.

@0x4007
Copy link
Member Author

0x4007 commented Dec 11, 2024 •
edited
Loading

@0x4007 Correct but the need of answering a question should not turn the pull-request into a draft no? Because the pull-request code itself won't change, that was my reasoning.

I have mixed feelings on this based on how we use comments in practice, but the way you frame it is logical and makes sense. Lets leave the pull request state as finalized in this situation then.

@gentlementlegen
Copy link
Member

All right then I will proceed with the following changes on the pull-request:

  • the PR will be converted to draft only if there are reviews in "changes requested" state
  • otherwise, only the reminder will be posted and no state change will occur on the pull-request

Sounds good?

@ubiquity-os-beta ubiquity-os-beta
Copy link 

+ Evaluating results. Please wait...

@ubiquity-os-beta ubiquity-os-beta
Copy link

 [ 79.83 WXDAI ] 

@gentlementlegen
Contributions Overview
ViewContributionCountReward
IssueTask175
IssueComment14.83
Conversation Incentives
CommentFormattingRelevancePriorityReward
Just to make sure, when the PR is converted back to a draft it s…
6.8
content:
  content:
    p:
      score: 0
      elementCount: 1
    a:
      score: 5
      elementCount: 1
  result: 5
regex:
  wordCount: 30
  wordValue: 0.1
  result: 1.8
0.834.83

 [ 75 WXDAI ] 

@0x4007
⚠️ Your rewards have been limited to the task price of 75 WXDAI.
Contributions Overview
ViewContributionCountReward
IssueSpecification144.52
IssueComment432.487
Conversation Incentives
CommentFormattingRelevancePriorityReward
- When a follow up occurs, the pull request should be turned int…
14.84
content:
  content:
    ul:
      score: 0
      elementCount: 1
    li:
      score: 0.5
      elementCount: 3
    p:
      score: 0
      elementCount: 5
    a:
      score: 5
      elementCount: 2
  result: 11.5
regex:
  wordCount: 62
  wordValue: 0.1
  result: 3.34
1344.52
YesWhen disqualified it needs to close the pull
0.65
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 9
  wordValue: 0.1
  result: 0.65
131.95
Let me try and think through the scenarios, ✅ means "success con…
8.72
content:
  content:
    p:
      score: 0
      elementCount: 7
    ul:
      score: 0
      elementCount: 1
    li:
      score: 0.5
      elementCount: 5
  result: 2.5
regex:
  wordCount: 129
  wordValue: 0.1
  result: 6.22
0.8322.428
It's not accepted so there is work they need to do, even if it m…
1.28
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 20
  wordValue: 0.1
  result: 1.28
0.632.304
I have mixed feelings on this based on how we use comments in pr…
2.15
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 37
  wordValue: 0.1
  result: 2.15
0.935.805

 [ 0.756 WXDAI ] 

@whilefoo
Contributions Overview
ViewContributionCountReward
IssueComment10.756
ReviewComment40
Conversation Incentives
CommentFormattingRelevancePriorityReward
What if the user is waiting for a PR review? in that case it doe…
1.44
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 23
  wordValue: 0.1
  result: 1.44
0.730.756
I think we should leave it at `0.34.3` same as in SDK be…
0
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 27
  wordValue: 0
  result: 0
0.7530
why `process.env`?
0
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 1
  wordValue: 0
  result: 0
0.530
Ideally I'd remove the package from plugin's dependencies and so…
0
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 34
  wordValue: 0
  result: 0
0.930
ok so because this is only used for testing locally?
0
content:
  content:
    p:
      score: 0
      elementCount: 1
  result: 0
regex:
  wordCount: 10
  wordValue: 0
  result: 0
0.330

@gentlementlegen gentlementlegen mentioned this issue Dec 12, 2024
Closed
@0x4007
Copy link
Member Author

0x4007 commented Dec 12, 2024 •
edited
Loading

We need to enable review comment incentives for everybody. Glad we have whilefoo for testing. @whilefoo perhaps you can open the pull and fix that.

It reminds me but we should also improve how it handles word count and long comments (this should be penalized/disincentivized heavily)

@rndquu rndquu removed this from Development Dec 13, 2024
@gentlementlegen gentlementlegen mentioned this issue Dec 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gentlementlegen gentlementlegen

Labels
Price: 75 USD Priority: 3 (High) Time: <1 Hour
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: pull-request state adjustments gentlementlegen/daemon-disqualifier
3 participants
@0x4007 @gentlementlegen @whilefoo