Npm backed repo #884
Npm backed repo #884
Conversation
|
Formatting check succeeded! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #884 +/- ##
============================================
+ Coverage 74.00% 74.07% +0.06%
Complexity 364 364
============================================
Files 592 593 +1
Lines 28444 28558 +114
Branches 3669 3688 +19
============================================
+ Hits 21049 21153 +104
- Misses 5592 5596 +4
- Partials 1803 1809 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/npm/NpmBackedRepository.java
Show resolved
Hide resolved
cqf-fhir-cql/src/test/java/org/opencds/cqf/fhir/cql/npm/NpmBackedRepositoryTest.java
Show resolved
Hide resolved
cqf-fhir-cql/src/main/java/org/opencds/cqf/fhir/cql/npm/NpmBackedRepository.java
Show resolved
Hide resolved
| try { | ||
| NpmPackage pkg = NpmPackage.fromFolder(Paths.get(folder, pkgName).toString(), false); | ||
| pkg.loadAllFiles(); | ||
| npmProcessor.getPackageManager().getNpmList().add(pkg); |
There was a problem hiding this comment.
Are we sure this is the right way to use the package manager? It seems like the package manager ought to be the thing loading the package?
| if (npmProcessor == null) { | ||
| // for some reason we require a 'base' sourceig... | ||
| // and this base *must be* R5 | ||
| org.hl7.fhir.r5.model.ImplementationGuide guide = new org.hl7.fhir.r5.model.ImplementationGuide(); |
There was a problem hiding this comment.
The reason an NpmProcessor needs an IG is to establish the set of dependencies in use, which determines the scope of contents available to that processor. If we know that scope to start with, it should be part of the initialization. In other words, it sets the project scope, and we need to understand what we want the relationship to project scope to be for this implementation. Is the NpmBackedRepository supposed to be used from the perspective of a particular IG, or is it supposed to enable Npm access across all IGs loaded in a repository?
|
|
||
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public <T extends IBaseResource> List<T> resolveByUrl(@Nonnull Class<T> clazz, String url) { |
There was a problem hiding this comment.
There doesn't seem to be version management here, what are the expectations for versioned and versionless references?
|
|
||
| if (hasUrl) { | ||
| return resources.stream() | ||
| .filter(r -> r.hasCanonicalUrl() && r.getCanonicalUrl().equals(url)) |
| continue; | ||
| } | ||
|
|
||
| List<T> pkgResources = getResourcesFromPkg(pkg, clazz); |
There was a problem hiding this comment.
This should be a lazy load on a cache miss. Packages may have literally 1000s of resources of any given type, and loading them all when we only need one is one of the most common scenarios here.
barhodes
left a comment
barhodes
left a comment
There was a problem hiding this comment.
What is the intended use of this class? Is it intended to be a Repository? If so, why is it not using the IRepository interface? If not, perhaps we should consider renaming this to avoid confusion.
No description provided.