Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: improvements #169
feat: improvements #169
Changes from 13 commits
b87f4ca
2b090cd
a3e1202
c9b92bc
7b15aa1
9aa4aec
99ef8ab
49e4220
1d3ee4c
d2746d1
37ca024
1ccbac3
2cbf1ed
1aa8f0c
f30dbb5
fe99961
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Confused on the purpose of this but if the full header is how we'll search for and parse the structured metadata then we can pull the org via the payload
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.
Should we add
if("pull_request" in ...)
as well and usepull_request.number
?I'm not 100% if payloads with
"pull_request"
in it always have"issue"
tooThere 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.
message.metadata?.name
will require we update the logger. I implementedpluginName
in this logger PR, is that whatname
represents here?No, name represents
className
notpluginName
sorry, I'm confused whatclassName
represents now is it the header by which to parse the structuredMetadata e.gask-llm-response
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 QA images I grabbed show that caller is always undefined no matter if it's
new Error
orthrow logger
You can take
caller
fromLogReturn
it should already have it and so shouldError
too.Personally I don't see the value in sticking the invoking fn name in the header used for search, but maybe that's short sighted of 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.
All of that code was some copy paste of some function you implemented in another project I think, forgot which one, will update that.
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's implemented in command-ask but I only added it last week and took it from this PR. It does look familiar tho I can agree with that, akin to the logger internals I thought at first. Possible I wrote it somewhere else but I can't recall
https://github.com/ubiquity-os-marketplace/command-ask/blame/eb5487d4879e48757ff758ed3fa90728469edcc1/src/handlers/comment-created-callback.ts#L63
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's from Ubiquibot V1 actually as far back as that
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.
Is this how it should be displayed? Why don't we just replace
<!-- --!>
with backticks and show the structured data with the header, is there a reason?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 think because it allows us to search for the metadata
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 thought search would be based on the components of the header not including the HTML comment syntax
<!--
at the start of the header. Bit harder to parse I guess without it, my point was duplicating the metadata but it's not a real problemIt's very unlikely we'll ever search by
caller
or byrevision
I think, what about you guys?If we used just the headers we could do something like this
UbiquityOS - <orgName> - <pluginName>
. First item being the name of the bot, just to make the query a bit more specific. IfclassName
===LogLevel
then that gives us a type of filter for search if we include it.Is there a spec for search/parse implementation yet?
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.
when we search we need to search for the comment syntax otherwise it can match other things, so we need to search for
<-- UbiquityOS ... <metadata> ... -->
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 can't resolve this but think it can be if double metadata being posted is not a problem