-
Notifications
You must be signed in to change notification settings - Fork 33
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: simplification incentives #288
base: development
Are you sure you want to change the base?
feat: simplification incentives #288
Conversation
will qa this in a bit |
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.
The pull request does not address the specified variable renaming requirements. According to the task specification, 'generateOnMissingPriceLabel' should be renamed to 'requirePriceLabel' and 'types' should be renamed to 'targets'. The current diff shows changes related to simplification incentives but does not include these required variable name changes.
the folly of my doing |
another bonus of the central payment system might be it making the tests and not bunched up in one place (especially process.issue.test.ts) actually understandable to new contributors, when i started out, the tests were really complex but i familiarized myself as time went on |
I suppose this is reading from #33 so this needs to be fixed. |
@0x4007 will get fixed by ubiquity-os-marketplace/daemon-pull-review#14 |
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.
The pull request does not address the specified variable renames from the task specification. The task requires renaming 'generateOnMissingPriceLabel' to 'requirePriceLabel' and 'types' to 'targets', but these changes are not present in the pull request diff. Instead, the diff shows changes related to adding simplification incentives functionality. Please update the pull request to include the required variable renames.
Ig the daemon pull review is still using the old version, the pr which fixed this got merged a minute ago edit: yep the update manifest and dist build workflow wasn't run |
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.
How does this play with the reward capping to the task value?
for assignee's they will each type of receive reward capped to the issue task reward like for example |
heres the qa ishowvel-org/.ubiquity-os#90 |
I don't think it makes sense to credit the assignee for reviewing their work. In fact, I am pretty sure the GitHub UI won't even allow for it. But strictly technically speaking the assignee isn't necessarily the pull author. The pull author is the one that can't review their work. I suppose you'll need to accommodate this edge case if the assignee is not the pull author then I suppose they could get review incentives in theory. Just never the pull author I'm pretty sure. |
the incentives that were being talked about were for simplification rewards and not review incentives, and i think it makes sense to credit the pull author and not the assignee because if we credit the assignee, there would be no way to get which respective pull request is for the assignee without their actual participation in being the pull requests author |
@gentlementlegen can this be merged, it has a passing qa ishowvel-org/.ubiquity-os#90 |
Resolves ubiquity-os/plugins-wishlist#33