Address Copilot review feedback on PR #4#5
Merged
Conversation
- example/basic userinfo: handle ParsePermission error explicitly with an "unknown" fallback instead of silently treating malformed permission strings as zero. - README permissions snippet: clarify that empty permission strings return (0, nil); only non-numeric input produces a real error. - websocket_test: drop the redundant TestMaxFramePayload_Value (the same constant is already pinned by TestMaxFramePayload_SanityCheck). - permissions.go godoc example: use ParsePermission so the snippet actually compiles against Member.Permissions (which is a string, not a numeric type). - rest_test: rewrite the BulkDeleteMessages / GetMessages / BanMember tests to drive the real RestClient methods through a recording http.RoundTripper, so the tests catch the case where a future refactor accidentally drops the clamping/validation logic. The "exactly at maximum" case now uses 100 IDs and asserts success instead of misleadingly using 101. - example/slash roll: convert the interaction ID's last byte to a decimal digit before applying %max so the result is the actual digit (0-9) rather than the ASCII code (e.g. 55 for '7').
There was a problem hiding this comment.
Pull request overview
Updates tests, docs, and examples to address prior review feedback, primarily by exercising real REST client methods in unit tests and clarifying permission parsing behavior.
Changes:
- Refactors REST tests to use a custom
RoundTripperto validate request paths/query/body without network calls. - Updates permission documentation/examples to use
ParsePermissionand clarifies the empty-string behavior. - Fixes deterministic “roll” example logic to use numeric digit value rather than ASCII byte code.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
websocket_test.go |
Removes a redundant max-frame-payload value test (coverage remains via existing sanity check). |
rest_test.go |
Adds a recording HTTP transport and rewrites tests to assert real request construction for clamping/validation. |
permissions.go |
Updates the quick-usage docs to recommend ParsePermission for Discord’s decimal-string permissions. |
example/slash/main.go |
Corrects deterministic roll calculation to interpret the last ID character as a decimal digit. |
example/basic/main.go |
Avoids ignoring ParsePermission errors and handles empty/malformed permission strings more explicitly in output. |
README.md |
Clarifies that empty permissions parse to (0, nil) and only malformed/out-of-range strings error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+28
| t.lastBody, _ = io.ReadAll(req.Body) | ||
| _ = req.Body.Close() |
There was a problem hiding this comment.
recordingTransport.RoundTrip ignores the io.ReadAll error (and also ignores any Close error). If the request body read fails, the test may silently proceed with an empty/partial recorded body. Please handle the read error (and optionally Close error) by returning it from RoundTrip so failures surface clearly.
Suggested change
| t.lastBody, _ = io.ReadAll(req.Body) | |
| _ = req.Body.Close() | |
| body, readErr := io.ReadAll(req.Body) | |
| closeErr := req.Body.Close() | |
| if readErr != nil { | |
| return nil, readErr | |
| } | |
| if closeErr != nil { | |
| return nil, closeErr | |
| } | |
| t.lastBody = body |
HiLleywyn
added a commit
that referenced
this pull request
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.