From ab2a6c8aeff8d5afbf8aea4cbcbdeab774750ed4 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 13:04:23 -0700 Subject: [PATCH] Address review feedback on PR #113 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix AllowInsecureHTTP doc comment: say '_test.go file in the same package' instead of 'export_test.go' (MINOR #1) - Remove dead u.Fragment = "" from redactURL: HTTP requests never carry fragments over the wire per RFC 7230 ยง5.1 (MINOR #2) - Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer that no network call should occur (NIT #3) --- github/client.go | 3 +-- github/client_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/github/client.go b/github/client.go index 41ecea8..227df06 100644 --- a/github/client.go +++ b/github/client.go @@ -155,7 +155,7 @@ type clientConfig struct { // environment variable. Without the env var set, the option is ignored // and a warning is logged. // -// For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. +// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate. func AllowInsecureHTTP() ClientOption { return func(cfg *clientConfig) { cfg.allowInsecureHTTP = true @@ -272,7 +272,6 @@ func redactURL(rawURL string) string { if u.RawQuery != "" { u.RawQuery = "" } - u.Fragment = "" return u.String() } diff --git a/github/client_test.go b/github/client_test.go index 6d2afa6..bb2f9f9 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -547,8 +547,8 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) { func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { // Verify case-insensitive scheme check (RFC 3986). - c := NewClient("tok", "HTTP://example.com") - _, err := c.doGet(context.Background(), "HTTP://example.com/test") + c := NewClient("tok", "HTTP://127.0.0.1:1") + _, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test") if err == nil { t.Fatal("expected error for uppercase HTTP scheme") } @@ -559,8 +559,8 @@ func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) { func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) { // Verify mixed case like "Http://" is also rejected. - c := NewClient("tok", "Http://example.com") - _, err := c.doGet(context.Background(), "Http://example.com/test") + c := NewClient("tok", "Http://127.0.0.1:1") + _, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test") if err == nil { t.Fatal("expected error for mixed-case HTTP scheme") }