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

fix: kratos proto server cmd method name bugfix (#2089) #2094

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

raw34
Copy link
Contributor

@raw34 raw34 commented Jun 10, 2022

  • add ucFirst function and unit test at cmd/kratos/internal/proto/server/server.go
  • fix the method name style bug

@Casper-Mars
Copy link
Contributor

This would not solve the issue #2089 . Issue #2089 is about camel case and snake case.

@raw34
Copy link
Contributor Author

raw34 commented Jun 11, 2022

This would not solve the issue #2089 . Issue #2089 is about camel case and snake case.

this commit just fix the part2 problem(which i affects me), like this:

# proto
service OrderItems {
	rpc CreateOrderItems (CreateOrderItemsRequest) returns (CreateOrderItemsReply);
......
}
# wrong service
......

func (s *OrderItemsService) Createorderitems(ctx context.Context, req *pb.CreateOrderItemsRequest) (*pb.CreateOrderItemsReply, error) {
	return &pb.CreateOrderItemsReply{}, nil
}

.....
# right service

......

func (s *OrderItemsService) CreateOrderItems(ctx context.Context, req *pb.CreateOrderItemsRequest) (*pb.CreateOrderItemsReply, error) {
	return &pb.CreateOrderItemsReply{}, nil
}

.....

@Casper-Mars
Copy link
Contributor

OH~Do you mean this PR is still working in progress?

@raw34
Copy link
Contributor Author

raw34 commented Jun 11, 2022

OH~Do you mean this PR is still working in progress?

the fact is i didn't find out the issue is about 2 problems, hah...

i can try to resolve the part1 problem some time later

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2022

Codecov Report

Merging #2094 (66bdcb4) into main (123fc1e) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2094      +/-   ##
==========================================
- Coverage   79.09%   79.03%   -0.06%     
==========================================
  Files          84       84              
  Lines        3683     3683              
==========================================
- Hits         2913     2911       -2     
- Misses        559      561       +2     
  Partials      211      211              
Impacted Files Coverage Δ
internal/context/context.go 97.01% <0.00%> (-2.99%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 123fc1e...66bdcb4. Read the comment docs.

@shenqidebaozi
Copy link
Sponsor Member

It seems that the problem is caused by this modification

@raw34
Copy link
Contributor Author

raw34 commented Jun 12, 2022

It seems that the problem is caused by this modification

yes, it is. the modification fix another bug, but trigger this bug. and my commit can fix it.

@shenqidebaozi
Copy link
Sponsor Member

Has this PR been completed?

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.

None yet

6 participants