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: Fixed getArgType reflection value logic #276

Merged

Conversation

ChangedenCZD
Copy link
Contributor

@ChangedenCZD ChangedenCZD commented Sep 6, 2021

What this PR does:
Fixed getArgType reflection value logic.

Source
1630564699562_2A57E06F-1539-4bec-9869-C7369CF69DCA

Demo
1630564706769_6D073A5C-CF4C-43f4-BB6B-DDF36991CC4B

Print
1630564711927_CBBCA459-7108-4afc-888F-A86FA1F971BC

Analyse
发现原版指向的类型是struct,如果使用t.Elem()会指向slice
测试了map,结果也是相似的

Conclusion
当getArgType的switch跑到default,且入参为指针时,很可能都会当成一般struct去处理

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@codecov-commenter
Copy link

Codecov Report

Merging #276 (07f2058) into master (8679ff8) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   68.87%   68.79%   -0.09%     
==========================================
  Files          26       26              
  Lines        2911     2903       -8     
==========================================
- Hits         2005     1997       -8     
  Misses        683      683              
  Partials      223      223              
Impacted Files Coverage Δ
request.go 61.50% <100.00%> (ø)
double.go 86.15% <0.00%> (-1.52%) ⬇️

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 8679ff8...07f2058. Read the comment docs.

Copy link
Contributor

@wongoo wongoo left a comment

Choose a reason for hiding this comment

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

LGTM

@zouyx zouyx merged commit 20ce834 into apache:master Sep 6, 2021
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.

5 participants