nHapi
nHapi copied to clipboard
nHapiNET
Include the optional LongName attribute in the XML encoded output.
@lmenaria I haven't had a chance to fully review the changes yet but I have noticed there are no unit tests, Can you add some unit test please, I'll hopefully get a chance to do a proper review soon.
yes, but how we can use parserOptions in those both methods,we need to pass or create private variable for it and assign?
what do you think?
@lmenaria I would say pass it down, have a look at how the PipeParser uses it.
Unit Test Results
1 372 tests 1 284 :heavy_check_mark: 4s :stopwatch: 3 suites 88 :zzz: 3 files 0 :x:
Results for commit 60548233.
:recycle: This comment has been updated with latest results.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@lmenaria a bunch of code style issues need fixing, see automated comments in PR.
Also I feel some of the xml doc comments need updating, from the scan I did anyway.
Also still no unit tests for these changes.
@lmenaria hi, thanks for that, however that wasn't the only static code analysis warning, if you look through the Files Changed you will see more easy to fix warnings.
Also there are still no unit tests to cover the new functionality, could you please add the appropriate unit tests.
Thank you.
Anything more need to do?
@laxmi-lal-menaria yes please, can you add appropriate unit tests for the new functionality.
@laxmi-lal-menaria looks like it failed again https://github.com/nHapiNET/nHapi/runs/7677435616?check_suite_focus=true
@laxmi-lal-menaria CI is still failing https://github.com/nHapiNET/nHapi/runs/7695823531?check_suite_focus=true
Testcases passed on Windows but not at ubuntu, I don't have any setup to fix that, if anyone can do it will be great help.
@laxmi-lal-menaria I fixed the issues with your PR, not sure if we need more/better tests (better as in obviouse what the test is doing).
@AMCN41R could you look at this? does it need anything more?
Looks ok to me... maybe want some extra xml parser tests for NOT including the long description?
@AMCN41R could you look at this? does it need anything more?
Looks ok to me... maybe want some extra xml parser tests for NOT including the long description?
Makes sense... will look to get those added.
after looking further into this, nhapi is way behind hapi in terms of hl7v2 xml support, I'll probably look to address this disparity first then come back to this PR.
@laxmi-lal-menaria okay I have rebased this PR based on what is now in master (makes this PR much smaller).
@AMCN41R @PhantomGrazzler could you give a quick once over - it's only a small PR.