Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

ExportToBoolPtr andRequiredParam#495

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

Merged
SamMorrowDrums merged 3 commits intomainfromlulu/export-utils
Jun 9, 2025
Merged

Conversation

LuluBeatson
Copy link
Contributor

  1. ExportToBoolPtr andRequiredParam functions from the github package.
  2. Clean up the type assertion inRequiredParam

@LuluBeatsonLuluBeatson changed the titleLulu/export-utilsExportToBoolPtr andRequiredParamJun 9, 2025
@LuluBeatsonLuluBeatson marked this pull request as ready for reviewJune 9, 2025 13:02
@CopilotCopilotAI review requested due to automatic review settingsJune 9, 2025 13:02
@LuluBeatsonLuluBeatson requested a review froma team as acode ownerJune 9, 2025 13:02
Copy link
Contributor

@CopilotCopilotAI left a 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 makes theToBoolPtr andRequiredParam helpers public and updates all call sites to use the exported versions, while simplifying the type assertion inRequiredParam.

  • ExportToBoolPtr and replace all uses oftoBoolPtr
  • ExportRequiredParam and update call sites, eliminating redundant lookups
  • Adjust documentation comment forToBoolPtr

Reviewed Changes

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

Show a summary per file
FileDescription
pkg/github/tools.goRenamedtoBoolPtr toToBoolPtr with doc comment
pkg/github/server_test.goUpdated test to useRequiredParam[string]
pkg/github/server.goRenamedrequiredParam toRequiredParam, cleaned up assertions
pkg/github/secret_scanning.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/search.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/repositories.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/pullrequests.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/notifications.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/issues.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/dynamic_tools.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
pkg/github/context_tools.goSwappedtoBoolPtr forToBoolPtr
pkg/github/code_scanning.goSwappedtoBoolPtr forToBoolPtr, updated requiredParam calls
Comments suppressed due to low confidence (2)

pkg/github/tools.go:148

  • Consider adding a unit test forToBoolPtr to verify it returns a non-nil pointer whose dereferenced value matches the input.
func ToBoolPtr(b bool) *bool {

pkg/github/server.go:79

  • Enhance this error message by including the actual value or type encountered (e.g., use%T with the actual argument) to aid debugging.
return zero, fmt.Errorf("parameter %s is not of type %T", p, zero)

@SamMorrowDrumsSamMorrowDrums merged commitcbcf29f intomainJun 9, 2025
16 checks passed
@SamMorrowDrumsSamMorrowDrums deleted the lulu/export-utils branchJune 9, 2025 13:11
@williammartin
Copy link
Collaborator

Just a couple of notes on this@LuluBeatson@SamMorrowDrums

I didn't exporttoBoolPtr becausegithub.ToBoolPtr is pretty janky. It's not a meaningful abstraction. It would likely be better for us to expose aghmcp.Annotations which uses types we care about and has a single adapter into the mcp server types.

Secondly, Istrongly encourage avoiding use of all the param parsing functions from this package. The jsonschema should be the source of truth. Using these takes the implementation away from where we should want it to go IMO.

That said, it's not like it's a huge cost to exporting and using these compared to where we are today. I just wanted to let you know my thinking on this.

SamMorrowDrums and LuluBeatson reacted with thumbs up emoji

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

Copilot code reviewCopilotCopilot left review comments

@SamMorrowDrumsSamMorrowDrumsSamMorrowDrums approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@LuluBeatson@williammartin@SamMorrowDrums

[8]ページ先頭

©2009-2025 Movatter.jp