moov-io / iso8583

A golang implementation to marshal and unmarshal iso8583 message.
https://moov.io
Apache License 2.0
353 stars 105 forks source link

fix for #220 bcd length encoding #221

Closed ddvk closed 1 year ago

ddvk commented 1 year ago

Here's is my proposition for fixing the bcd length encoding. This also fixes the variable encoding where an odd length was encoded with the next even number.

The problem I had with bcd is that there is no way to find out whether the length of original data was odd or even, that's why the Encoder interface, Encode return type change.

I couldn't come up with a more elegant solution.

adamdecaf commented 1 year ago

Thanks for opening this PR! Can you update the godoc comments to mention what that integer represents?

ddvk commented 1 year ago

the godoc of the Encode method?

adamdecaf commented 1 year ago

Yep, thanks.

adamdecaf commented 1 year ago

cc @alovak

Can we fix and amend that typo? @ddvk

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 93.47% and project coverage change: +0.01 :tada:

Comparison is base (79404f0) 73.18% compared to head (4c3426a) 73.19%.

:exclamation: Current head 4c3426a differs from pull request most recent head c55f697. Consider uploading reports for the commit c55f697 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #221 +/- ## ========================================== + Coverage 73.18% 73.19% +0.01% ========================================== Files 41 41 Lines 1995 1996 +1 ========================================== + Hits 1460 1461 +1 Misses 326 326 Partials 209 209 ``` | [Impacted Files](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [encoding/binary.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvYmluYXJ5Lmdv) | `30.00% <0.00%> (ø)` | | | [encoding/ebcdic1047.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvZWJjZGljMTA0Ny5nbw==) | `76.47% <66.66%> (ø)` | | | [encoding/ascii.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvYXNjaWkuZ28=) | `89.47% <100.00%> (ø)` | | | [encoding/bcd.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvYmNkLmdv) | `100.00% <100.00%> (ø)` | | | [encoding/bertlv.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvYmVydGx2Lmdv) | `90.90% <100.00%> (-0.40%)` | :arrow_down: | | [encoding/ebcdic.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvZWJjZGljLmdv) | `100.00% <100.00%> (ø)` | | | [encoding/hex.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvaGV4Lmdv) | `87.09% <100.00%> (ø)` | | | [encoding/lbcd.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZW5jb2RpbmcvbGJjZC5nbw==) | `100.00% <100.00%> (ø)` | | | [field/binary.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvYmluYXJ5Lmdv) | `60.67% <100.00%> (ø)` | | | [field/bitmap.go](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvYml0bWFwLmdv) | `74.19% <100.00%> (ø)` | | | ... and [11 more](https://codecov.io/gh/moov-io/iso8583/pull/221?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

alovak commented 1 year ago

@ddvk thanks for raising the issue and your contributions. You helped us to realize that there is an issue with field packing/unpacking. I'm closing this PR as the issue is going to be addressed by this PR: https://github.com/moov-io/iso8583/pull/227

ddvk commented 1 year ago

sure, thanks a lot.