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

feat: Add FWAV, INFO, ITAV, ITBD and XPCD balance types #157

Merged
merged 6 commits into from
Aug 6, 2024

Conversation

DannyvdSluijs
Copy link
Contributor

This PR solves #156

@DannyvdSluijs DannyvdSluijs changed the title feat: Add FWAV, INFO, ITAV, ITDB and XPCD balance types feat: Add FWAV, INFO, ITAV, ITBD and XPCD balance types Aug 1, 2024
Copy link
Collaborator

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

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

Looks like a good start, but we miss unit tests for it. You can add a new pair of xml and json files in test/data, to test all cases at once.

@DannyvdSluijs
Copy link
Contributor Author

Thanks for the feedback. I've added test.

One think to be cautious are the other tests that started to fail due to unsupported balance types also being in those files.
I think it also showed a different problem as the test/data/camt052.v6.xml file had a balance with a DtTm element (In the Balance type the Dt field is of a type DateAndDateTimeChoice. For the mentioned file there was a DtTm sub element which isn't supported in code resulting in the test always failing as it was doing a fallback to now when creating the DateTime object.
For the scope of this PR I opted to correct the regression file to match the code.

Let me know what you think and if any changes are needed.

@PowerKiKi PowerKiKi merged commit 127adca into genkgo:master Aug 6, 2024
7 checks passed
@PowerKiKi
Copy link
Collaborator

Thank you !

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.

2 participants