Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Steve/jsonschemas #14

Open
wants to merge 20 commits into
base: gaby/fhir_mapping
Choose a base branch
from

Conversation

playtime222
Copy link
Contributor

No description provided.

Copy link
Contributor

@gabywh gabywh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some generally applicable comments I didn't repeat everywhere (e.g. codableConcept -> codeableConcept) but they do need applying throughout.
$id inconsistent with filenames.
"sex <-> gender" confused here

playtime222 and others added 9 commits April 12, 2021 00:54
…/index FHIR search results for processing. Added json/dict access helpers. Added FhirInfoReader to extract data from FHIR search results. Added mappers for PODs.
… to JsonParser. Fixed spelling mistakes and typos. Temporarily removed catch block in PatientImmunizationBuilder tp ensure errors are visible.
# Conflicts:
#	examples/EUVAC/cose_sign.py
#	examples/EUVAC/fhir2qr.py
#	examples/EUVAC/fhir_query.py
#	examples/EUVAC/min_data_set.py
#	examples/EUVAC/tests/component/test_immu_parser.py
#	examples/EUVAC/tests/unit/test_min_data_set.py
Copy link
Contributor

@gabywh gabywh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach of using a JsonParser class.

I also like the Builder approach.

My impression is that this is a re-write to use POD and have high coupling / low cohesion on type throughout the code. In order to avoid such coupling, the data was encapsulated behind class types. I won't explain this further in this comment. This encapsulation is now broken and I see a lot of edits here that have little functional value but are simply switching from one programming paradigm (OO) to an earlier programming paradigm (procedural). Given that this is a small demo project, I am comfortable (even if not happy) with this switch.

Some coding violations have crept in which have added nothing to the functional working of this project: one example of this is the now non-standard (illegal) module names which for some reason have been re-named to follow the C# convention although this source code is Python (orig: "from disclosure_level import DisclosureLevel" has become "from DisclosureLevel import DisclosureLevel" which not only adds nothing in terms in functionality but also violates Python naming/coding standards), another (no newline at end of file) is even flagged by the github UI etc.

That said, if you really want to persist with regressing to a primarily procedural approach with its higher coupling and lower cohesion, then you should consider using something like NamedTuple for the POD and not abuse "class", which strongly implies OO.

Two tips:

  1. format with black https://pypi.org/project/black/
  2. PEP8 has the definitive coding guidelines. Several linters can check conformity, PyCharm will also by default warn you for non-compliance. Format with black will guarantee PEP8 compliance.

found = self.__resolveReference(ref)
self._result.practitioners[ref] = found

def __resolveReference(self, item):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed

Copy link
Contributor Author

@playtime222 playtime222 Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reply to header comment:

About 2 hours ago I already decided you wouldn't like amount of classes involved for a POC, so I slowly merged the builders/mappers and far less code.
I did lost unit tests with the last commit though as they worked on the POD/OO version.
That's first on the TODO list now.
Closely followed by your 2 tips which I can do more mechanically as my brain fades today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also removed as it was dead code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants