Skip to content

fix: address code review feedback for GRAPE provider#45

Merged
Amygos merged 6 commits intograpefrom
claude/sub-pr-42-again
Mar 5, 2026
Merged

fix: address code review feedback for GRAPE provider#45
Amygos merged 6 commits intograpefrom
claude/sub-pr-42-again

Conversation

@Claude
Copy link
Copy Markdown

@Claude Claude AI commented Mar 5, 2026

Addresses review feedback on PR #42 to improve error handling, thread safety, and documentation accuracy in the GRAPE provider implementation.

Thread safety improvements:

  • Replaced mutex-based caching with sync.Once in getProvisioningServerID() and getEndpointsURL() to prevent race conditions and duplicate API calls under concurrent load
  • Separated debug mutex from cache fields for cleaner synchronization

Error handling enhancements:

  • makeHawkRequest() now properly handles io.ReadAll errors and returns structured APIError for HTTP 4xx/5xx responses
  • GrapeDevice.Register() distinguishes transport-level errors (DNS, connection, timeout) from API errors, only wrapping the former as connection_to_remote_provider_failed
// Callers can now inspect API errors
err := client.RegisterDevice(mac, url)
var apiErr grape.APIError
if errors.As(err, &apiErr) {
    // Handle specific HTTP status codes
}

Documentation fixes:

  • Clarified that GRAPE uses Hawk authentication (HMAC-based), not OAuth, in README and environment variable descriptions
  • Updated API manual to use errors.As() instead of type assertion for error checking
  • Changed Go version requirement to reference go.mod instead of hardcoded value

URL handling:

  • NewClient() normalizes BaseURL to ensure trailing slash, preventing malformed URLs during endpoint concatenation

@Claude Claude AI assigned Claude and Amygos Mar 5, 2026
@Claude Claude AI mentioned this pull request Mar 5, 2026
@Claude Claude AI changed the title [WIP] Add support for Gigaset Redirect and Provisioning Environment fix: address code review feedback for GRAPE provider Mar 5, 2026
@Amygos Amygos marked this pull request as ready for review March 5, 2026 09:35
@Amygos Amygos force-pushed the claude/sub-pr-42-again branch from 5ab653a to 19f8c25 Compare March 5, 2026 10:07
@Amygos Amygos merged commit a73823c into grape Mar 5, 2026
2 checks passed
@Amygos Amygos deleted the claude/sub-pr-42-again branch March 5, 2026 10:08
Sign up for free to 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.

2 participants