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
Error log in notification #176
base: master
Are you sure you want to change the base?
Error log in notification #176
Changes from all commits
cea946f
0cda62f
0feaab3
e6a5b3b
81dd235
40f6e87
fe9d189
e6e39ae
c971f14
4e981c8
b045a27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
do we really need this? I think the types that we have cover all of our needs, and we can handle any strings that we need to show but don't care about through the open enum escape hatch.
Also, when are adding them all, you shouldn't keep the
open_enum
, it defeats the purpose.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 am not 100% sure all of them are in there (the documentation is not great). This work was already done for buildkite-ui so it felt very reasonable to just import it, it was more work to think about which ones are actually needed.
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 makes a reply different from a message or replies so far? Why not use the
make_message
function? We already handle replies in threads by calling this functionThere 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.
A reply does not have a
thread_ts
, because it is attached to a real message and uses thethread_ts
that that message returns when sent. I could maybe use the types for messages but make surethread_ts
isNone
, and later edit it.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.
We already have ways to handle threads. That's what the
handler
argument of the make_message function is for. The last changes done to the way we handle threads were in #165.Check also
monorobot/lib/action.ml
Lines 397 to 402 in 9f3ce75
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 saw what the handler did. From memory it was unable to send a message.
Currently monorobot does not have this behaviour of sending a message to a thread at the same time as the message is written. Messages are send to threads on subsequent notifications, when the thread_ts is already known.