Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 4e04de5 | Previous: d607bf6 | Ratio |
|---|---|---|---|
Benchmark_Panic |
0.001554 ns/op 0 B/op 0 allocs/op |
0.0006579 ns/op 0 B/op 0 allocs/op |
2.36 |
Benchmark_Panic - ns/op |
0.001554 ns/op |
0.0006579 ns/op |
2.36 |
This comment was automatically generated by workflow using github-action-benchmark.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #892 +/- ##
==========================================
+ Coverage 67.80% 69.16% +1.35%
==========================================
Files 152 157 +5
Lines 10042 10519 +477
==========================================
+ Hits 6809 7275 +466
- Misses 2910 2911 +1
- Partials 323 333 +10 ☔ View full report in Codecov by Sentry. |
hwbrzzl
left a comment
There was a problem hiding this comment.
Amazing PR 👍 Left some questions, and have another question: I found a lot of methods are repeated with the testing.http module, can they be merged in some ways?
| var err error | ||
| switch method { | ||
| case http.MethodGet: | ||
| resp, err = s.request.Clone().Get(server.URL) |
There was a problem hiding this comment.
Why does every request need to call .Clone()? Is .Clone required in the real situation? I think if you want to eliminate parameter effects, facades.Http() can not be a singleton.
There was a problem hiding this comment.
Not necessary in the real situation, but since test cases share same request instance, it is required
|
Could you please create other PRs for goravel/goravel and goravel/docs? |
Yeah, already in progress |
hwbrzzl
left a comment
There was a problem hiding this comment.
Thanks, just left three nitpicks.
* add basic methos * add http facade * chore: update mocks * add more methods to response * chore: update mocks * remove timeout for each request * chore: update mocks * parseURL properly * add test cases for request * add test cases for response * fix:lint * remove unused methods * chore: update mocks * add more test cases * remove testJson * update client config * Update http/client/request_test.go --------- Co-authored-by: Wenbo Han <hwbrzzl@gmail.com>
* add basic methos * add http facade * chore: update mocks * add more methods to response * chore: update mocks * remove timeout for each request * chore: update mocks * parseURL properly * add test cases for request * add test cases for response * fix:lint * remove unused methods * chore: update mocks * add more test cases * remove testJson * update client config * Update http/client/request_test.go --------- Co-authored-by: Wenbo Han <hwbrzzl@gmail.com>
📑 Description
Closes goravel/goravel#565
@coderabbitai summary
✅ Checks