fix: allow posting on pull-requests #65
Conversation
There was a problem hiding this comment.
Otherwise it works fine:
ubiquibot-whilefoo-testing/testing#13
ubiquibot-whilefoo-testing/testing#10
src/comment.ts
Outdated
| const metadata = message.metadata | ||
| ? { | ||
| message: message.metadata.message, | ||
| stack: message.metadata.stack || message.metadata.error?.stack, | ||
| caller: message.metadata.caller || message.metadata.error?.stack?.split("\n")[2]?.match(/at (\S+)/)?.[1], | ||
| } | ||
| : { ...message }; |
There was a problem hiding this comment.
If I use logger.ok("something", { first: 1, second: 2 }), the metadata I provided won't be in the comment metadata. Was this intended from the beginning?
There was a problem hiding this comment.
No, this is a mistake. Do you mean I introduced that during refactoring or was that like this before?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah it was like that from beginning but I've only noticed now
src/comment.ts
Outdated
| @@ -5,6 +5,7 @@ import { PluginRuntimeInfo } from "./helpers/runtime-info"; | |||
| import { sanitizeMetadata } from "./util"; | |||
|
|
|||
| const HEADER_NAME = "UbiquityOS"; | |||
| const lastCommentId = { reviewCommentId: null as number | null, issueCommentId: null as number | null }; | |||
There was a problem hiding this comment.
I'm concerned that this can lead to some bugs. If I understand correctly Cloudflare workers sometimes run multiple requests in the same instance of the worker , so if two requests were posting comments it would cause some of them to get comment id from another request.
There was a problem hiding this comment.
That's a good point. To fix it I believe this should be then stored in a new class to make sure it is not shared between instances, but then it would mean that postComment could not be called via a function but rather would need to come from some object, which might be inconvenient? Might have to move postComment inside of the context or something similar.
There was a problem hiding this comment.
@whilefoo Wrapped the comment posting inside of a class, and exposed it from the context:
da526ff
I believe this is important to address because I actually have noticed the behavior you mentioned
ubiquity/ubiquity-dollar#985 (comment)
This will require changes from all the plugins sadly.
There was a problem hiding this comment.
Maybe Context should become a class in the future
Resolves #63
QA: Meniole/plugin-template#3