feat: support TXP Addenda05 strings #1683
Conversation
| // Validate 80-byte limit for TXP addenda | ||
| if len(paymentInfo) > 80 { | ||
| return nil, ErrInvalidTXPFormat | ||
| } |
There was a problem hiding this comment.
String lengths should always use utf8.RuneCountInString
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for TXP Addenda05 strings, which is a valuable addition. The implementation includes parsing, serialization, and comprehensive unit tests that cover various scenarios, including NACHA examples and edge cases. The code is well-structured. I have a couple of suggestions to improve maintainability by removing an unused field and making the tests more consistent and concise. Overall, great work.
| // TaxInformationIDNumber is a unique identifier for the tax information | ||
| TaxInformationIDNumber string |
There was a problem hiding this comment.
The TaxInformationIDNumber field is defined in the TXP struct but it is not populated during parsing in ParseTXP or used during serialization in String(). This appears to be unused code. To improve maintainability and avoid confusion, this field should be removed.
| if result.TaxIdentificationNumber != tt.expected.TaxIdentificationNumber { | ||
| t.Errorf("TaxIdentificationNumber: expected %s, got %s", tt.expected.TaxIdentificationNumber, result.TaxIdentificationNumber) | ||
| } | ||
| if result.TaxPaymentTypeCode != tt.expected.TaxPaymentTypeCode { | ||
| t.Errorf("TaxPaymentTypeCode: expected %s, got %s", tt.expected.TaxPaymentTypeCode, result.TaxPaymentTypeCode) | ||
| } | ||
| if result.Date != tt.expected.Date { | ||
| t.Errorf("Date: expected %s, got %s", tt.expected.Date, result.Date) | ||
| } | ||
| if len(result.TaxAmounts) != len(tt.expected.TaxAmounts) { | ||
| t.Errorf("TaxAmounts length: expected %d, got %d", len(tt.expected.TaxAmounts), len(result.TaxAmounts)) | ||
| } else { | ||
| for i, expectedAmount := range tt.expected.TaxAmounts { | ||
| if result.TaxAmounts[i].AmountCents != expectedAmount.AmountCents { | ||
| t.Errorf("TaxAmounts[%d].AmountCents: expected %s, got %s", i, expectedAmount.AmountCents, result.TaxAmounts[i].AmountCents) | ||
| } | ||
| if result.TaxAmounts[i].AmountType != expectedAmount.AmountType { | ||
| t.Errorf("TaxAmounts[%d].AmountType: expected %s, got %s", i, expectedAmount.AmountType, result.TaxAmounts[i].AmountType) | ||
| } | ||
| } | ||
| } | ||
| if result.TaxpayerVerification != tt.expected.TaxpayerVerification { | ||
| t.Errorf("TaxpayerVerification: expected %s, got %s", tt.expected.TaxpayerVerification, result.TaxpayerVerification) | ||
| } |
There was a problem hiding this comment.
The testify/require package is already imported and used in TestTXPToStringRoundTrip. For consistency and to make tests more concise, you can replace these manual if checks with require.Equal(). This simplifies the test and provides more detailed failure messages. A similar refactoring can be applied to the other test functions in this file that use t.Errorf.
require.Equal(t, tt.expected, result)Addenda: TXP validation improvements
No description provided.