Nested NPM package support #1149
Conversation
…pendency loading for npm package manager class
…n checks, and enhanced testing
|
Ready for review. Resolution for #847 @ktarasenko feel free to validate this logic with any testing you have locally |
|
Thanks, will give it a try! |
| @@ -3,4 +3,6 @@ | |||
| dependencies { | |||
| implementation project(':cql-to-elm') | |||
| implementation group: 'com.google.code.gson', name: 'gson', version: '2.9.1' | |||
| implementation group: 'org.apache.commons', name: 'commons-compress', version: '1.20' | |||
There was a problem hiding this comment.
I don't see where you are using these dependencies
| } | ||
|
|
||
| @Override | ||
| public void logMessage(String message) { |
| @@ -40,7 +39,7 @@ public Library readLibrary(InputStream stream) throws FHIRFormatError, IOExcepti | |||
| VersionConvertor_40_50 versionConvertor_40_50 = new VersionConvertor_40_50(new BaseAdvisor_40_50()); | |||
| return (Library) versionConvertor_40_50.convertResource(res); | |||
| } else if (VersionUtilities.isR5Ver(version)) { | |||
| return (Library) new JsonParser().parse(stream); | |||
| return (Library) new org.hl7.fhir.r5.formats.JsonParser().parse(stream); | |||
There was a problem hiding this comment.
Yes. This is needed to avoid org.objectweb.asm.ClassTooLargeException: Class too large: org/hl7/fhir/r5/formats/JsonParser. See this PR for more information on why that is required.
| import org.hl7.fhir.convertors.conv40_50.VersionConvertor_40_50; | ||
| import org.hl7.fhir.exceptions.FHIRException; | ||
| import org.hl7.fhir.r4.formats.FormatUtilities; | ||
| import ca.uhn.fhir.model.primitive.IdDt; |
There was a problem hiding this comment.
what's the difference between this and IdType?
There was a problem hiding this comment.
IdDt is not model-specific
| private static final Logger logger = LoggerFactory.getLogger(NpmPackageManager.class); | ||
| private final ImplementationGuide sourceIg; | ||
| private final FilesystemPackageCacheManager fspcm; | ||
| private final List<NpmPackage> npmList; |
|
|
||
| try { | ||
| // userMode indicates whether the packageCache is within the working directory or in the user home | ||
| fspcm = new FilesystemPackageCacheManager(true, ToolsVersion.TOOLS_VERSION); |
There was a problem hiding this comment.
Can I ask you to go an extra mile and add a way to inject FilesystemPackageCacheManager (but keep the existing manager as the default) ?
This will solve two problems:
- allow to provide a fake package manager to have better tests (like the test that verifies that dependencies were actually loaded, similar to what i did here )
- use the same class with more android friendly version FilesystemPackageCacheManager
There was a problem hiding this comment.
Applied in latest commit
Just seeing if this passes CI build for now. Still needs cleanup and hardening