Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: http and route #198

Merged
merged 33 commits into from
Aug 16, 2023
Merged

refactor: http and route #198

merged 33 commits into from
Aug 16, 2023

Conversation

devhaozi
Copy link
Member

@devhaozi devhaozi commented Jun 19, 2023

Closes goravel/goravel#206
Closes goravel/goravel#93
Closes goravel/goravel#187

📑 Description

Refactor http and route.

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

ℹ Additional Information

none.

@devhaozi
Copy link
Member Author

devhaozi commented Jun 19, 2023

把Gin移除以后Auth包那边的测试没法运行了,缺少http.Background方法的完整实现。
怎么解决比较好呢?

cc @hwbrzzl.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Jun 20, 2023

将 Auth 的测试与 Route/Http 解耦吧?如果需要,Auth 直接使用 net/http 发起请求。

# Conflicts:
#	go.mod
#	go.sum
#	http/gin_request.go
#	http/gin_response.go
#	http/service_provider.go
#	route/gin_test.go
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.90% 🎉

Comparison is base (0fc9eca) 63.46% compared to head (387f600) 65.36%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
+ Coverage   63.46%   65.36%   +1.90%     
==========================================
  Files         145      136       -9     
  Lines        9023     8423     -600     
==========================================
- Hits         5726     5506     -220     
+ Misses       2907     2532     -375     
+ Partials      390      385       -5     
Files Changed Coverage Δ
foundation/container.go 51.61% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, clearer!

route/route.go Outdated Show resolved Hide resolved
http/context.go Outdated Show resolved Hide resolved
@devhaozi devhaozi requested a review from hwbrzzl June 30, 2023 16:12
http/context.go Outdated Show resolved Hide resolved
@devhaozi devhaozi enabled auto-merge (squash) June 30, 2023 16:36
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to remove so many files. 👍

http/context.go Outdated Show resolved Hide resolved
foundation/application_test.go Outdated Show resolved Hide resolved
http/context.go Outdated Show resolved Hide resolved
http/context.go Outdated Show resolved Hide resolved
http/context.go Outdated Show resolved Hide resolved
http/service_provider.go Outdated Show resolved Hide resolved
route/route.go Show resolved Hide resolved
@devhaozi
Copy link
Member Author

@devhaozi 「预期是想直接绑定到goravel.http和goravel.route上,这样就不用配置了,安装好在config/app.go中添加一下直接就用。」 这句话应该如何理解啊?绑定什么?app.go 中添加什么?

image
app.go 中添加gin的ServiceProvider

I think it's a good idea. 👍

Maybe it can't be done because can't register routes. I'm going to try to fallback to the original way.

@devhaozi devhaozi self-assigned this Aug 11, 2023
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! It seems to be fine, I'll check out this branch and try it locally, could you please add the new configuration to the PR description?

http/service_provider.go Outdated Show resolved Hide resolved
@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 11, 2023

Sorry, I saw the configuration in goravel/gin.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Aug 13, 2023

FYI, I'm testing goravel/http-middleware.

@devhaozi devhaozi merged commit 56b504a into master Aug 16, 2023
15 checks passed
@devhaozi devhaozi deleted the haozi-gin branch August 16, 2023 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants