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

Return subfields in unpack error #313

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

meparle
Copy link
Contributor

@meparle meparle commented Jul 12, 2024

Adding the capability to return subfield IDs from a composite type field within an UnpackError when there is an error unpacking a subfield.

It is necessary to provide this further detail for some card schemes as part of the format error information.

Adding a field FieldIDs to UnpackError to return the top level field ID plus an ID for each of however many levels of nested subfields error during unpacking.

The field FieldID could be deprecated once FieldIDs is present and sufficient notice given. Happy to add a deprecation notice if you would like.

@meparle meparle requested a review from alovak as a code owner July 12, 2024 10:03
Copy link
Contributor

@alovak alovak left a comment

Choose a reason for hiding this comment

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

what I suggest is to keep error wrapping you added, but instead of collecting the IDs in the wrapper methods, create a helper, that will iterate over the errors and extract all field IDs like this:

		fieldIDs := []string{}
		var unpackError *mooverrors.UnpackError
		for {
			if errors.As(err, &unpackError) {
				fieldIDs = append(fieldIDs, unpackError.FieldID)
				err = unpackError.Unwrap()
			} else {
				break
			}
		}

		fmt.Println(fieldIDs) // [3 2] - the same we expect in the tests

With such an approach, we keep UnpackError as it is, add error wrapping into a composite field and maybe a helper method without breaking changes.

What do you think?

errors/errors.go Show resolved Hide resolved
field/composite.go Outdated Show resolved Hide resolved
message.go Outdated Show resolved Hide resolved
@alovak alovak merged commit dfac6bd into moov-io:master Jul 15, 2024
8 checks passed
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