Support acknowledgment entries ack & atx #327 #331
Conversation
Initial ACK Support
create batchATX and batchATX_test CTX and ACK adjustments
ACk and AXT testing adjustments
read and write test change SetOriginalTraceNumber argument from int to string
add ATX write and read test minor adjustments for ACK write tes
Update README.md for ACK and ATX
| // Position 21-22 of underlying Individual Name are reserved blank space for CTX " " | ||
| func (ed *EntryDetail) SetCTXReceivingCompany(s string) { | ||
| func (ed *EntryDetail) SetCATXReceivingCompany(s string) { |
There was a problem hiding this comment.
This will break any users who had the old method, is that okay?
There was a problem hiding this comment.
That's correct, that would have to update the methods to use the new methods. I can leave the old methods and we can deprecate them later. I'm not sure how many clients are using the CTX* functions. This may be a good way to find out. Let me know if you want me to leave the CTX methods and deprecate later.
There was a problem hiding this comment.
Can we ask on the mailing list? cc @wadearnold
It's likely not a big deal, but we should double check.
There was a problem hiding this comment.
Sure, the company the updates would know right away. It's not like they would update production. I'll send out a note to the mailing list.
There was a problem hiding this comment.
We can ask the mailing list. I don't understand the motivation for the renaming of CTX -> CATX ?
There was a problem hiding this comment.
To be a bit more relevant to what it supports both CTX and ATX EntryDetail, although I don't really like it.
I can leave CTX and both CTX EntryDetail and ATX EntryDetail can use it. I don't like that either.
We can create ATX specific functions. In this case specific to adding it in EntryDetail, I don't like that either.
So I chose the one I thought was best from 3 solutions I didn't like. If it's agreed that another above is more preferable or some other solution is better I can make the adjustment. Just let me know.
go.sum
Outdated
| @@ -1,7 +1,12 @@ | |||
| github.com/VividCortex/gohistogram v1.0.0 h1:6+hBz+qvs0JOrrNhhmR7lFxo5sINxBCGXrdtl/UvroE= | |||
There was a problem hiding this comment.
Can we drop these files from the PR? I'd like to update dependencies in their own PR's.
| mockBatch.AddEntry(mockACKEntryDetail()) | ||
| mockBatch.GetEntries()[0].AddAddenda(mockAddenda05()) | ||
| if err := mockBatch.Create(); err != nil { | ||
| log.Fatal(err) |
There was a problem hiding this comment.
This should be t.Fatal, right?
There was a problem hiding this comment.
The way that is currently defined throughout ach it could be a panic(err), but I'd prefer it log. At some point we could clean up those test still using panic(err)
There was a problem hiding this comment.
Ah, that's fine. log.Fatal calls do break control flow away from the current goroutine anyway and testing picks up those to mark as failed.
There was a problem hiding this comment.
I would prefer in the library that we always return an error and not dictate a logging structure. We can have stronger opinions in our own services that consume the library.
There was a problem hiding this comment.
I can make it panic(err)
There was a problem hiding this comment.
This is test code, so why not t.Fatal? That doesn't panic -- it just stops the test.
There was a problem hiding this comment.
gah! Sorry i can't read a code snippet! I thought that was the factory in the batch.go
There was a problem hiding this comment.
yes it's creating a mockBatch, it's not actually running a test, but tests use it to create a mockBatch. no different then *entryDetail creates an entryDetail. There is some additional logic to panic(err) if batch creates fail. There is no point in continuing with the test if that batch fails. I like it the way it is except I'd prefer to log. If you want to change the library to use t.Fatal, please do. I'm not sure it is necessary or needed. Or create an issue and we can address the entire library under that issue, if it makes sense too.
remove go.mod and go.sum
replace go/mod and go.sum
Add back removed CTX functions to be deprecated in the future
No description provided.