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

Composites with private bitmaps #229

Closed FacundoMora closed 1 year ago

FacundoMora commented 1 year ago

Hi Everyone!,

A few month ago me and my team encountered with a peculiar type of field, that implied a structure like this: [LL BCD Length] + [3 bytes bitmap] + [Subfields]

In this structure we were missing two features:

As you already know, in a previous release, a change about arbitrary size bitmaps made by @alovak took place, in this changes our first feature needed to solve this situation was finished.

So now, we bring the second feature, where we can use this bitmap as a private bitmap of a composite 👏

To achieve this we decided to reutilize most of the logic in iso8583.Message, specifically the way that uses its bitmap to find the message fields for packing and unpacking. Obviously we had to modify a bit of code to make sense on a composite context.

PD: Yes, iso8583.Message could be replaced as a composite given that their behaviour are pretty the same, but with @alovak agree on leave it for a future potential change, so that these changes do not become of great impact.

Finally, a minor change we made was that we moved the spec validation location inside the SetSpec along with the ContructSubfields call. We made this because we thought this makes more sense given that this two things have to run if the spec is changed.

I look forward for your comments on this, thanks!

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 85.18% and project coverage change: +0.83 :tada:

Comparison is base (49dc79f) 73.10% compared to head (2060db4) 73.94%.

:exclamation: Current head 2060db4 differs from pull request most recent head d126ec4. Consider uploading reports for the commit d126ec4 to get more accurate results

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #229 +/- ## ========================================== + Coverage 73.10% 73.94% +0.83% ========================================== Files 41 41 Lines 1997 2069 +72 ========================================== + Hits 1460 1530 +70 + Misses 327 325 -2 - Partials 210 214 +4 ``` | [Impacted Files](https://app.codecov.io/gh/moov-io/iso8583/pull/229?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [field/spec.go](https://app.codecov.io/gh/moov-io/iso8583/pull/229?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvc3BlYy5nbw==) | `66.66% <ø> (ø)` | | | [field/composite.go](https://app.codecov.io/gh/moov-io/iso8583/pull/229?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-ZmllbGQvY29tcG9zaXRlLmdv) | `82.81% <85.18%> (+1.08%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/moov-io/iso8583/pull/229/indirect-changes?src=pr&el=tree-more&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

@FacundoMora hey! Thanks for the PR! Great job! I'm reviewing it.

alovak commented 1 year ago

@FacundoMora It looks good, just some tiny changes are needed and we can merge it.