-
Notifications
You must be signed in to change notification settings - Fork 29
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
PR: correct discrepancies with the old bot #55
PR: correct discrepancies with the old bot #55
Conversation
old method was also detecting PR first comment as spec
otherwise tests fail as some files are created in runtime
It wasn't just h5 missing from permit comment, there were other html tags missing too. It was because of removal of all new lines from comments. New line is needed to identify some html tags for example Thing also shows a new concerns though. The bot scores 1 to tags not html listed in config. Is this expected? |
Defaults should be set to |
Question: |
From my understanding, any comment within the pull request should have its relevance set to |
Good catch on this task. Why don't you address that in a new pull? Also provide a time estimate. Consider that the main work will probably be to adjust the prompt. |
@gentlementlegen I see tests failing here, and on also the development branch https://github.com/ubiquibot/conversation-rewards/actions/runs/10076181691 . Probably because of your config changes yesterday? In the failing test, it started to change your github role between contributor and collaborator. |
@EresDev I'll run lots of tests and come back to you. |
@EresDev I just ran it against ubiquity/pay.ubq.fi#195 and it seems some comments are not picked up still. And the specification comment doesn't seem to be displayed properly as well, can you confirm? Since this is urgently needed I opened a PR against your repo here: EresDevOrg#1 |
Latest QA (tests conducted with my fixes as well):
|
fix: fixing display and collection errors
I have merged your PR. I believe everything is fixed for this PR? @gentlementlegen |
@EresDev on the issues I mentioned it should be yes. Actually right now ubiquibot-test is running will all these changes. Needing code review now! |
369d614
into
ubiquity-os-marketplace:development
multipliers: | ||
- select: [ISSUE_SPECIFICATION] | ||
relevance: 1 | ||
- select: [PULL_AUTHOR] | ||
relevance: 1 | ||
- select: [PULL_ASSIGNEE] | ||
relevance: 1 | ||
- select: [PULL_COLLABORATOR] | ||
relevance: 1 | ||
- select: [PULL_CONTRIBUTOR] | ||
relevance: 1 |
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.
Sets these to relevance 1? This is not clear to me.
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 requirement was to set the relevance of issue specifications, and PR comments to 1. To implement that I added something called "Fixed Relevance" and you can specify it in config. You can add fixed relevance to any comment type, and if you do so, fixed relevance will take precedence and that comment type will not be sent to OpenAI for evaluation, and the fixed relevance will be applied to that comment type. What you see in the readme file above are fixed relevance of 1 being applied to issue specifications and PR comments.
userExtractor: | ||
redeemTask: true |
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.
What is this? We should just make it a single property without nesting probably.
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 nesting is die to userExtractor
being its own module and made it clearer that this module is the only one using these variables.
redeemTask
means the tasks are redeemable e.g. the reward can be collected.
* Checks if the comment is made by a human user, not empty, and not a command. | ||
*/ | ||
_checkEntryValidity(comment: (typeof IssueActivity.prototype.allComments)[0]) { | ||
return comment.body && comment.user?.type === "User"; | ||
return comment.body && comment.user?.type === "User" && comment.body.trim()[0] !== "/"; |
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.
I'm surprised that empty comments have been found in testing. I didn't know that is possible.
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.
It should not be but according to GitHub types body
can eventually be undefined
.
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.
Fix set specs as 1
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.
/start
Resolves #26
QA
What can be improved?
Lots of file changes just for prettier formatting. Probably need some stricter check on push or commit so that all team members send formatted code.