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: multiply conv reward with priority for higher reward #176

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

Conversation

ishowvel
Copy link

Resolves #175

@ishowvel
Copy link
Author

/help

Copy link

Available Commands

Command Description Example
/help List all available commands. /help
/allow Allows the user to modify the given label. /allow @user1 label
/query Returns the user's wallet, access, and multiplier information. /query @UbiquityOS
/start Assign yourself to the issue. /start
/stop Unassign yourself from the issue. /stop
/wallet Register your wallet address for payments. /wallet ubq.eth

@ishowvel
Copy link
Author

ishowvel commented Nov 1, 2024

heres the qa

@ishowvel
Copy link
Author

ishowvel commented Nov 1, 2024

@gentlementlegen please review this 🙏

Copy link

@ishowvel, this task has been idle for a while. Please provide an update.

@gentlementlegen gentlementlegen changed the base branch from development to main November 3, 2024 10:42
@gentlementlegen gentlementlegen changed the base branch from main to development November 3, 2024 10:42
@gentlementlegen
Copy link
Member

@ishowvel You should add Jest tests to your pull-request.

Here is my latest QA:
image

The decimals should not look like this. Also, I am confused about the total here. Can you walk me through the calculations?

@ishowvel
Copy link
Author

ishowvel commented Nov 3, 2024

After the total formatted reward is calculated, the priority level is multiplied to the reward for that specific comment, this all happens in the formatting evaluation.
Then after the reward is passed to content evaluation to calc and multiply it with the relevance for that specific comment.

The only change here is adding a multiplication of the priority level to the formatting reward calculation

@ishowvel
Copy link
Author

ishowvel commented Nov 3, 2024

the total itself is not multiplied but the specification and comment incentives are

@ishowvel
Copy link
Author

ishowvel commented Nov 3, 2024

for jest testing, no mock result has ever contained the "priority" key. Can you help me find a way to make the mock results for this?
all results starting at the formatting module would need to be changed

@0x4007
Copy link
Member

0x4007 commented Nov 3, 2024

Just copy the priority level names? Our config is public you can see all of them .ubiquity-os repo

@ishowvel
Copy link
Author

ishowvel commented Nov 3, 2024

Just copy the priority level names? Our config is public you can see all of them .ubiquity-os repo

There are 100's and 100's of results

@0x4007
Copy link
Member

0x4007 commented Nov 3, 2024

There's five priority levels

@ishowvel
Copy link
Author

ishowvel commented Nov 3, 2024

I am talking about the results, each result object would need to be edited to have an extra priority key and the total and reward values would need to be edited

@ishowvel
Copy link
Author

ishowvel commented Nov 5, 2024

Done!

@ishowvel
Copy link
Author

ishowvel commented Nov 5, 2024

All tests that failed, failed because of not supporting this feature. So I just added support in all of them

@ishowvel
Copy link
Author

ishowvel commented Nov 5, 2024

@gentlementlegen can you please review these changes? Thank you!

@ishowvel
Copy link
Author

ishowvel commented Nov 6, 2024

Any updates?

@ishowvel
Copy link
Author

ishowvel commented Nov 7, 2024

@0x4007 can you review this as you have created the specification, gentlemengen may be busy

@gentlementlegen
Copy link
Member

@ishowvel Sorry I have been extremely busy yes, I'll try to have a look today. In the meantime please fix the conflicts.

Copy link

@ishowvel, this task has been idle for a while. Please provide an update.

@ishowvel
Copy link
Author

ishowvel commented Nov 9, 2024

Currently out for a university, will work on this task in a few hours

@ishowvel
Copy link
Author

ishowvel commented Nov 9, 2024

idk why the tests keep failing even without the changes present in this pull request, i made a branch and force reset it to this repo's dev branch, heres the test

@gentlementlegen
Copy link
Member

@ishowvel tests don't pass because the import types don't match, I think @octokit/rest is imported when it should be @octokit/core within the tests.

@ishowvel
Copy link
Author

@gentlementlegen @octokit/core is imported, this has become a major blocker for me as i cant debug my other pr without working tests

@ishowvel
Copy link
Author

tests even fail on a fresh clone of this repository

@gentlementlegen
Copy link
Member

@ishowvel merge the development branch into this one.

@ishowvel
Copy link
Author

Tests are passing 🎉 🎉 🎉

Copy link

@ishowvel, this task has been idle for a while. Please provide an update.

@ishowvel
Copy link
Author

@gentlementlegen can you please review this

@ishowvel
Copy link
Author

Do reminders exist for reviewers like the ones for assignee

@gentlementlegen
Copy link
Member

@ishowvel I tried to run the plugin with your pull-request and this is what I got
https://github.com/Meniole/text-conversation-rewards/actions/runs/11868228402/job/33077194863#step:2:173

Does it work on your side?

@ishowvel
Copy link
Author

ishowvel commented Nov 16, 2024

From what I have tried using @octokit/core makes the build work but makes the tests fail and @octokit/rest makes the build fail and test work

@ishowvel
Copy link
Author

My code changes do not produce this bug as you can see here
https://github.com/ubiquity-os-marketplace/text-conversation-rewards/actions/runs/11769854878
The latest merge is also failing because of the same reason

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.

Boosting All Rewards Based On Priority
3 participants