change for adding multiple isa loops per file #12
Conversation
|
One other thing. We have same fake files to for our unit tests. Could you add a new file to the tests that have multiple ISA loops so that the new code is well tested? |
|
Sure...let me see what I can come up with |
|
Sorry, but I haven't touched this project in a while and I did another cleanup pass and caused a minor merge conflict. I am done making changes now. |
|
I took a look at the changes as well. Things looks good to me. I agree that adding an additional test for files with multiple ISAs is a good idea just to make sure everything is still working as expected. |
|
I'm about to upload a new version with the test for multiple ISAs shortly.
Thx for allowing me to take a stab at it.
D
…On Fri, Feb 8, 2019 at 2:22 PM angelaszek ***@***.***> wrote:
I took a look at the changes as well. Things looks good to me. I agree
that adding an additional test for files with multiple ISAs is a good idea
just to make sure everything is still working as expected.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AtQOLzqYVy6U-4dGEpVWXMGajHOIUvhPks5vLdybgaJpZM4av_RC>
.
|
|
I have updated the unit tests and fixed a bug identified during the testing. Please let me know what you think or if I need to add any more to it |
|
I'm going to take a look at it soon, but might not get to it until next week. Couple of things:
Thanks for all your work and if all goes well we can release this soon. |
|
Ok, the CI builds really were not working. I actually fixed them this time and verified by running against your PR. There are not failing checkstyle errors. Please merge master into your branch and you will see them on the next commit. Sorry about all this. I never actually set this project up correctly before this. All should be good now. |
|
thx...i'll do it now...i was cleaning up the noted findings anyway.
D
…On Fri, Feb 8, 2019 at 5:30 PM Chuck May ***@***.***> wrote:
Ok, the CI builds really were not working. I actually fixed them this time
and verified by running against your PR. There are not failing checkstyle
errors. Please merge master into your branch and you will see them on the
next commit.
Sorry about all this. I never actually set this project up correctly
before this. All should be good now.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AtQOL4x218atq3RG61g3xC5IohtnAKADks5vLgimgaJpZM4av_RC>
.
|
…g the final IEA segment (also put test in to verify it is there now)
| // Determine if we have started a new loop | ||
| loopId = getMatchedLoop(line.split(Pattern.quote(separators.getElement().toString())), previousLoopId); | ||
| if (loopId != null) { | ||
| if( loopId.equals( _definition.getLoop().getXid() ) ) { | ||
| if( null!=currentLoopId ) { |
There was a problem hiding this comment.
currentLoopId != null is the standard way to do this check
There was a problem hiding this comment.
I can make the change.
Here are the changes I suggest to allow for multiple ISA loops inside a single X12 request/response