search

nHapiNET / nHapi

nHapi is the .Net port of the original Java project HAPI.
Mozilla Public License 2.0
273 stars 155 forks source link

Include the optional LongName attribute in the XML encoded output. #308

Closed laxmi-lal-menaria closed 1 year ago

laxmi-lal-menaria commented 2 years ago

Added a property which include the Long Name in Encoded XML

laxmi-lal-menaria commented 2 years ago

How it can merge to master?

milkshakeuk commented 2 years ago

@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.

milkshakeuk commented 2 years ago

@lmenaria have you seen this yet?

laxmi-lal-menaria commented 2 years ago

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?

milkshakeuk commented 2 years ago

@lmenaria I would say pass it down, have a look at how the PipeParser uses it.

github-actions[bot] commented 2 years ago

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.

milkshakeuk commented 2 years ago

@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.

milkshakeuk commented 2 years ago

@lmenaria still a bunch of static analysis warnings which need fixing:

image
laxmi-lal-menaria commented 2 years ago

Fixed this warning.

milkshakeuk commented 2 years ago

@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.

laxmi-lal-menaria commented 2 years ago

Anything more need to do?

milkshakeuk commented 2 years ago

Anything more need to do?

@laxmi-lal-menaria yes please, can you add appropriate unit tests for the new functionality.

milkshakeuk commented 2 years ago

@laxmi-lal-menaria did you notice the failed build?

milkshakeuk commented 2 years ago

@laxmi-lal-menaria failed again.

milkshakeuk commented 2 years ago

@laxmi-lal-menaria looks like it failed again https://github.com/nHapiNET/nHapi/runs/7677435616?check_suite_focus=true

milkshakeuk commented 2 years ago

@laxmi-lal-menaria CI is still failing https://github.com/nHapiNET/nHapi/runs/7695823531?check_suite_focus=true

laxmi-lal-menaria commented 2 years ago

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.

milkshakeuk commented 1 year ago

@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).

milkshakeuk commented 1 year ago

@AMCN41R could you look at this? does it need anything more?

AMCN41R commented 1 year ago

@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?

milkshakeuk commented 1 year ago

@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.

milkshakeuk commented 1 year ago

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.

milkshakeuk commented 1 year ago

@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.