Export ToBoolPtr and RequiredParam
#495
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR makes the ToBoolPtr and RequiredParam helpers public and updates all call sites to use the exported versions, while simplifying the type assertion in RequiredParam.
- 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 | Renamed toBoolPtr to ToBoolPtr with doc comment |
| pkg/github/server_test.go | Updated test to use RequiredParam[string] |
| pkg/github/server.go | Renamed requiredParam to RequiredParam, cleaned up assertions |
| pkg/github/secret_scanning.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/search.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/repositories.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/pullrequests.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/notifications.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/issues.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/dynamic_tools.go | Swapped toBoolPtr for ToBoolPtr, updated requiredParam calls |
| pkg/github/context_tools.go | Swapped toBoolPtr for ToBoolPtr |
| pkg/github/code_scanning.go | Swapped toBoolPtr for ToBoolPtr, 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)
|
Just a couple of notes on this @LuluBeatson @SamMorrowDrums I didn't export Secondly, I strongly 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