Refactor P_BMM implementation #68
Conversation
…r_defs need to be done still in validator/creator
…ecifications-ITS-BMM again
…e tests, works ok for now
Codecov Report
@@ Coverage Diff @@
## master #68 +/- ##
============================================
+ Coverage 69.11% 70.06% +0.94%
- Complexity 4635 5283 +648
============================================
Files 554 616 +62
Lines 16153 18103 +1950
Branches 2261 2585 +324
============================================
+ Hits 11164 12683 +1519
- Misses 3932 4255 +323
- Partials 1057 1165 +108
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
A lot more readable than the previous implementation, but difficult for me to say there are no issues anywhere. I couldn't find any anyway. Most things have tests, and the validation and conversion of the openehr rm 102 are all passing and have been checked manually.
Some documentation could be improved, especially the BmmSchemaConverter. Could you also go over the TODO's? Some can be removed it seems to me. Apart from that this looks good 👍
| // | ||
| // if (!archetype.getTranslations().isEmpty()) { | ||
| // language.put("translations", archetype.getTranslations()); | ||
| // } |
| public class UniqueSchemaIdValidation implements BmmValidation { | ||
| @Override | ||
| public void validate(BmmValidationResult validationResult, BmmRepository repository, MessageLogger logger, PBmmSchema schema) { | ||
| //TODO: |
There was a problem hiding this comment.
Please either remove this validation, or describe what has to be done. I assume there is as reason why you haven't implemented this, but it is not clear to me
There was a problem hiding this comment.
I'll remove it - it's not possible to have non-unique schema ids in this stage, the repository will overwrite any you try to add.
|
|
||
| createModelsByClosureAndVersion(result); | ||
|
|
||
| return result; |
There was a problem hiding this comment.
I believe there is still some spaghetti here. Validation and converter steps are a bit intertwined, some steps are hidden within other steps, and documentation could be better. It is a lot better compared to what we had previously though, so kudos for that! If we do run into validation errors now, it should undoubtedly be easier to spot what they are.
If you could add a little more documentation right here of what each step does, should already add a lot more clarity. For example about what the preprocessin, descendantsCalculator and createModelsByClosureAndVersion does, or what is actually returned
There was a problem hiding this comment.
Yes there is. Some of the validations can only be easily run after part of the conversion, that's why it's intertwined. Could be fixed, but lots of complicated code, so I left it. I'll see if I can write some comments :)
| @JsonIgnore | ||
| public PBmmBaseType getTypeRef() { | ||
| if(typeDef == null && type != null) { | ||
| if(type.length() == 1) {//TODO!!!!: ?!?!?! Probably a parameter such as "T" |
There was a problem hiding this comment.
Confusing TODO ;) Does something have to be fixed here, or are you just unsure when this case would be met?
TODO:
happy flow integration tests with quite a lot of BMM models
Discuss with @cnanjo . Hi :)
Implement generic ODIN serializer with Jackson
Reintroduce a cyclic include check to solve possible infinite loop.
implement ancestor_defs instead of just ancestor - note that this did not work in the deprecated P_BMM implementation. Converted to BMM 2.0, so some information in the model is lost
change use of case insensitive treemaps into case insensitive order preserving maps.
update readme with documentation about BMM and ODIN
deprecate the old P_BMM implementation
error flow tests
probably some cleanup after review
Optional, or for a later change:
Note: the base branch for this PR is the construct_example_json branch. Was much easier to do this with the ExampleJsonInstanceGenerator in place, because it does quite a lot of testing of the BMM. That one should probably be merged first into master before this one.