Skip to content

Add list notifications tool - #297

Closed
martinajir wants to merge 5 commits into
mainfrom
martinajir-list-notifications
Closed

Add list notifications tool#297
martinajir wants to merge 5 commits into
mainfrom
martinajir-list-notifications

Conversation

@martinajir

@martinajirmartinajir commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

Description

Add a tool to list notifications

DoL exercise to add list notifications tool for github MCP server with with @krzd, @leachj, @geramirez using copilot agent mode

Screenshots

Screenshot 2025-04-22 at 14 33 46
Screenshot 2025-04-22 at 14 33 58

Closes: N/A -

@martinajirmartinajir changed the title WIP: List notifications toolAdd list notifications toolApr 22, 2025
@martinajir
martinajir marked this pull request as ready for review April 22, 2025 13:35
CopilotAI review requested due to automatic review settings April 22, 2025 13:35
@martinajir
martinajir requested a review from a team as a code ownerApril 22, 2025 13:35

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 a tool to list GitHub notifications, providing new endpoints and tests for retrieving notifications with customizable paging and filtering.

  • Introduces a new notifications toolset in pkg/github/tools.go.
  • Implements the ListNotifications handler in pkg/github/notifications.go with supporting tests in pkg/github/notifications_test.go.
  • Updates pkg/github/server.go to include a new helper for optional boolean parameters and documents the change in README.md.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
FileDescription
pkg/github/tools.goAdds a notifications toolset and registers it with the tool group.
pkg/github/server.goAdds OptionalBoolParamWithDefault helper but contains logic issues.
pkg/github/notifications_test.goProvides tests for the notifications tool functionality.
pkg/github/notifications.goImplements the ListNotifications tool.
README.mdUpdates documentation with the new notifications tool information.

Comment threadpkg/github/server.go Outdated
@martinajir
martinajir requested review from krzd and leachjApril 22, 2025 13:36
Comment threadpkg/github/server.go Outdated
@martinajir
martinajir requested a review from CopilotApril 22, 2025 15:46

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 a new tool to list GitHub notifications along with its implementation, tests, and documentation updates.

  • Introduces a notifications toolset with read functions.
  • Implements the ListNotifications tool in the GitHub package.
  • Provides tests for notifications behavior and updates the README documentation.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

FileDescription
pkg/github/tools.goAdds a new toolset for notifications with read capabilities.
pkg/github/notifications.goImplements the notifications listing functionality.
pkg/github/notifications_test.goAdds tests ensuring proper behavior of the notifications tool.
README.mdUpdates documentation to include the new notifications tool details.
Comments suppressed due to low confidence (1)

pkg/github/notifications_test.go:117

  • [nitpick] Consider adding assertions for additional fields such as the URL to further improve test coverage of notification properties.
assert.Equal(t, *tc.expectedResponse[i].Subject.Title, *notification.Subject.Title)

@martinajir
martinajir requested a review from krzdApril 22, 2025 15:46
require.NoError(t, err)
assert.Equal(t, len(tc.expectedResponse), len(returnedNotifications))
for i, notification := range returnedNotifications {
assert.Equal(t, *tc.expectedResponse[i].ID, *notification.ID)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot is somewhat right here with:

[nitpick] Consider adding assertions for additional fields such as the URL to further improve test coverage of notification properties.

@martinajir
martinajir removed the request for review from leachjApril 23, 2025 12:17
@geramirez

Copy link
Copy Markdown

I wonder if the models could use this in tandem with the get issues and prs tools.

@SamMorrowDrums

Copy link
Copy Markdown
Collaborator

I am closing this in favour of #225 as discussed. It was a great piece of work, but I had to pick one of the two, and the earlier one was more complete. Thanks for understanding, and once the new notifications tools are released, please feed provide feedback or PRs to make them work better!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@martinajir@geramirez@SamMorrowDrums@krzd@williammartin