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

Update message unimarshaller functions #291

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Conversation

mfdeveloper508
Copy link
Contributor

@mfdeveloper508 mfdeveloper508 requested a review from alovak as a code owner October 5, 2023 13:38
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (ec6abff) 74.91% compared to head (a257d96) 74.75%.

❗ Current head a257d96 differs from pull request most recent head 1103771. Consider uploading reports for the commit 1103771 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   74.91%   74.75%   -0.17%     
==========================================
  Files          43       43              
  Lines        2436     2452      +16     
==========================================
+ Hits         1825     1833       +8     
- Misses        389      396       +7     
- Partials      222      223       +1     
Files Coverage Δ
message.go 74.13% <71.42%> (+0.02%) ⬆️
field/composite.go 78.70% <57.14%> (-1.43%) ⬇️

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

field/composite.go Outdated Show resolved Hide resolved
message.go Outdated Show resolved Hide resolved
message_test.go Outdated Show resolved Hide resolved
@alovak
Copy link
Contributor

alovak commented Oct 5, 2023

It looks good! Would you please add table tests for Marshal/Unmarshal something like this: https://github.com/moov-io/iso8583/blob/2f44c9f5c9bf72927313d35e4dfaa478711943fb/exp_test.go?

message.go Outdated Show resolved Hide resolved
@mfdeveloper508
Copy link
Contributor Author

I will append exp_test.go in next pr

@@ -153,7 +153,7 @@ func (f *String) Unmarshal(v interface{}) error {
}

func (f *String) Marshal(v interface{}) error {
if v == nil || reflect.ValueOf(v).IsZero() {
if v == nil || (!reflect.ValueOf(v).CanInt() && reflect.ValueOf(v).IsZero()) {
Copy link
Contributor

@alovak alovak Oct 5, 2023

Choose a reason for hiding this comment

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

When we add keepzero we should check if value is 0 (int/*int) for field.String, then we should assign it.

Also, it may be similar to some other types. Not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right
the code is exception for having int value zero

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.

3 participants