-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: returnDataToKernel #44
base: development
Are you sure you want to change the base?
fix: returnDataToKernel #44
Conversation
return contentWithoutFootnotes; | ||
} | ||
|
||
export function removeCautionMessages(content: string): string { | ||
const cautionRegex = />[!CAUTION]\n> This issue may be a duplicate of the following issues:\n((> - \[[^\]]+\]\([^)]+\)\n)+)/g; |
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 are these checks? Seems like wrong approach to be doing this. I would expect you should be able to select the right data in the first place and process less comments with a better strategy, although I don't understand the full context of this process here.
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.
These checks are in place to avoid generating embeddings for comment or issue bodies that include footnotes or caution messages.
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.
Just ignore every bot comment is my point
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.
Yes, but when the user updates an issue, it includes footnotes and a caution statement. Should these be propagated to the database as well? I think bot actions are ignored by the kernel.
} | ||
if (data && data.length > 0) { | ||
this.context.logger.info("Comment already exists"); |
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.
You should be able to just throw this, then it logs and halts function execution.
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 not sure I understand what you're suggesting. Are you asking if I should log a fatal error instead of throwing a new one to halt the function execution?
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.
@gentlementlegen rfc
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.
You can just throw logger.fatal
so you get the log displayed and the error caught on a higher level of execution.
Resolves #43
GITHUB_TOKEN
in compute.yml