Cleanup ADL14Converter code #648
Conversation
| @@ -153,15 +136,18 @@ private List<String> findUnnecessaryCodes(CObject cObject, Map<String, Archetype | |||
| return result; | |||
| } | |||
|
|
|||
| /** | |||
| * Replace old id's in term definition with the new codes | |||
There was a problem hiding this comment.
One line 156-161, it seems that there is a bug, because the newTerm variable is never used, so I expect that needed to be used on line 161 instead just 'term'. But I don't want to change any functionality in this PR. Might look at this later.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #648 +/- ##
============================================
+ Coverage 71.96% 72.01% +0.04%
+ Complexity 7071 7069 -2
============================================
Files 671 671
Lines 23056 23037 -19
Branches 3745 3745
============================================
- Hits 16592 16589 -3
+ Misses 4717 4703 -14
+ Partials 1747 1745 -2 ☔ View full report in Codecov by Sentry. |
| * Sets the default occurrences with ADL 1.4 rules, if not explicitly set in a given Archetype. | ||
| * Useful for conversion to ADL 2, where the default values are different, and it is good to start | ||
| * Sets the default occurrences with ADL 1.4 rules ({1..1}), if not explicitly set in a given Archetype. | ||
| * Useful for conversion to ADL 2, where the default values are different ({0..*}), and it is good to start |
There was a problem hiding this comment.
they are not {0..*} default - they are whatever is in the reference model for that property. Which can be one of at least 0..1, 1..1, 0..* or 1..*.
| @@ -31,20 +31,9 @@ public void setDefaults(Archetype archetype) { | |||
| } | |||
|
|
|||
| private void correctItemsMultiplicities(CObject cObject) { | |||
| for(CAttribute attribute:cObject.getAttributes()) { | |||
| // according to the specification, the following lines must be added. | |||
There was a problem hiding this comment.
Has this been removed from the specification? Otherwise it is a valid comment.
| public ADL2ConversionResult() { | ||
|
|
||
| /* Empty construction for Jackson parsing */ |
There was a problem hiding this comment.
You have moved javadoc comments, which are actually converted to documentation on the build, to an internal comment. That does not seem like a good idea, as the javadoc will no longer list that this constructor should not be used.
| * | ||
| * @param existingRepository the existing repository to use | ||
| */ | ||
| public void setExistingRepository(InMemoryFullArchetypeRepository existingRepository) { |
There was a problem hiding this comment.
I do not think it is not a good idea to remove this. It may be unused in Archie at the moment, but is necessary if you are converting OPTs, for example. It is part of the public API of Archie.
There was a problem hiding this comment.
It is used at least here, and without it, this functionality cannot be built: https://github.com/nedap/archetype-languageserver/blob/a379ff5c81760d0601a9d18589585cd5cbc6d844/src/main/java/com/nedap/openehr/lsp/repository/ADL14ConvertingStorage.java#L51
| correctItemsCardinality(archetype.getDefinition()); | ||
| List<String> unnecessaryCodes = findUnnecessaryCodes(archetype.getDefinition(), | ||
| archetype.getTerminology().getTermDefinitions().get(archetype.getOriginalLanguage().getCodeString())); | ||
| convert(archetype.getDefinition()); | ||
| if(previousConversionApplier != null) { |
There was a problem hiding this comment.
You can actually start a conversion without having a previous conversion. It should thus be part of the API, or the readme needs an update on how to initialize this as an empty converter. Making the API call more difficult.
So why was this null-check removed?
There was a problem hiding this comment.
The archetype languege server does not support storing previous conversion, so does never set a previous conversion applier, for example. Please do not remove.
There was a problem hiding this comment.
Good start for a cleanup. however, I think you removed a bit too much - this harms the public API in several places. At least:
- if no previous conversion has been added, it does not make sense to upply a previous conversion applier. You could in that case initialize it to something that does not perform any operation, if you do not want a null check.
- the existing repository is very useful to have, if you want to convert in steps instead of all in one go. It should not be removed. For example, if you have converted 10 archetypes before, and someone supplies a new ADL 1.4 repository, you can use it to convert only the new file - and keep the previously converted ADL 2 results as input.
See individual comments for some more minor things.
|
Please check against at least the archetype language server that you can make it work again with the API changes. Unused code in Archie does not mean unused code, if it is part of the public API. |
Before I'm going to add anything new to the ADL14Converter code, I wanted to cleanup the code a little.
if(CObject object.....for example