- Notifications
You must be signed in to change notification settings - Fork908
Add "Mark notification as done" functionality#270
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add "Mark notification as done" functionality#270
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2075a36 toedc1adaCompareThere 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.
Pull Request Overview
This PR adds the "Mark Notification as Done" functionality to the notifications tooling. The changes include:
- Adding a new tool registration in pkg/github/server.go.
- Implementing the MarkNotificationDone function in pkg/github/notifications.go to mark a specific notification thread as done.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/github/server.go | Registers the new tool for marking a notification as done. |
| pkg/github/notifications.go | Implements MarkNotificationDone with parameter parsing and API call. |
Comments suppressed due to low confidence (1)
pkg/github/notifications.go:260
- [nitpick] Consider renaming the parameter 'getclient' to 'getClient' for consistency with similar functions and to improve readability.
func MarkNotificationDone(getclient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) {@rfearing as having too many tools might become an issue, maybe it'd be worth exploring the possibility of merging some of these tools into one. It seems that marking notifications might be a good candidate to having just one and using the parameters to decide what to do exactly. And the same goes for the getters. In general, we should probably explore providing higher level APIs instead of having a 1:1 mapping with the underlying API. We are already doing that in some tools with a positive result. In any case, these are just my two cents. |
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.
Nice work! 💖
6eaa8d3 intonotifications-toolingUh oh!
There was an error while loading.Please reload this page.
Surprised this was merged without addressing@juruen 's comment. Probably it makes more sense for the two mark notification tools to be one with an enum for what state to change it to. Can consider as a follow-up though. |
Oh, sorry I didn't realise this was merged into a branch! |
@SamMorrowDrums,@juruen I targeted@sridharavinash's branch and that PR has not yet been merged @sridharavinash I'm happy to invest some time into looking into@juruen's comment but in your branch. |
@rfearing thank you, really appreciate this too - I was just worried it landed into main without a final pass. There's a bit of a rebase involved to get it into main too :-D Bu also happy to take a look at the next branch, I just missed it wasn't a merge to main! |
Uh oh!
There was an error while loading.Please reload this page.
Add
MarkNotificationAsDoneto notifications toolingNote: Target branch is
notifications-tooling#225Feature added
Technical implementation
mark_notification_readthreadId,WithNumberran into some issues, so I kept the param as a string. I'm open to suggestions.