- Notifications
You must be signed in to change notification settings - Fork1k
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
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.
- Export
ToBoolPtrand replace all uses oftoBoolPtr - Export
RequiredParamand update call sites, eliminating redundant lookups - Adjust documentation comment for
ToBoolPtr
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/tools.go | RenamedtoBoolPtr toToBoolPtr with doc comment |
| pkg/github/server_test.go | Updated test to useRequiredParam[string] |
| pkg/github/server.go | RenamedrequiredParam toRequiredParam, cleaned up assertions |
| pkg/github/secret_scanning.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/search.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/repositories.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/pullrequests.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/notifications.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/issues.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/dynamic_tools.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
| pkg/github/context_tools.go | SwappedtoBoolPtr forToBoolPtr |
| pkg/github/code_scanning.go | SwappedtoBoolPtr forToBoolPtr, updated requiredParam calls |
Comments suppressed due to low confidence (2)
pkg/github/tools.go:148
- Consider adding a unit test for
ToBoolPtrto 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
%Twith the actual argument) to aid debugging.
return zero, fmt.Errorf("parameter %s is not of type %T", p, zero)cbcf29f intomainUh oh!
There was an error while loading.Please reload this page.
Just a couple of notes on this@LuluBeatson@SamMorrowDrums I didn't export 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. |
ToBoolPtrandRequiredParamfunctions from the github package.RequiredParam