[GitHub] [sling-org-apache-sling-feature-cpconverter] anchela commented on a change in pull request #95: SLING-10579: fix package types in emitter events

[GitBox <gi...@apache.org>]

anchela commented on a change in pull request #95:
URL: https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/95#discussion_r662813430



##########
File path: src/main/java/org/apache/sling/feature/cpconverter/vltpkg/DefaultPackagesEventsEmitter.java
##########
@@ -123,10 +123,13 @@ public void startPackage(@NotNull VaultPackage vaultPackage) {
         idOutputLine.put(id, String.format("%s,%s,%s,,,\n",
             paths.peek(),
             hierarchy.peek(),
-            detectPackageType(vaultPackage)));
+            "PACKAGE_TYPE"));

Review comment:
       would it make sense to create a constant for that and attach a comment that this is a place-holder-type. 

##########
File path: src/main/java/org/apache/sling/feature/cpconverter/vltpkg/PackagesEventsEmitter.java
##########
@@ -33,4 +34,6 @@
 
     void endSubPackage();
 
+    default void finalizePackage(@NotNull PackageId id, @NotNull VaultPackage vaultPackage){};

Review comment:
       i would like to suggest to add a bit of javadoc explaining how/when the method should be used.
   in PackagesEventsEmitterTest.java  i see that it's called between emitter.startSubPackage and emitter.endSubPackage..... hm.... so in can (must?) be called for the main-package and sub-packages?
   minor nitpicking: the ; at the end is unnecessary
   

##########
File path: src/main/java/org/apache/sling/feature/cpconverter/vltpkg/DefaultPackagesEventsEmitter.java
##########
@@ -112,40 +116,36 @@ public void end() {
     }
 
     @Override
-    public void startPackage(@NotNull VaultPackage vaultPackage) {
-        PackageId id = vaultPackage.getId();
+    public void startPackage(@NotNull PackageId id, @NotNull VaultPackage vaultPackage) {
         Dependency[] dependencies = vaultPackage.getDependencies();
         paths.add(vaultPackage.getFile().getAbsolutePath());
         hierarchy.add(id);
         current = vaultPackage;
 
-        packages.add(getDepOnlyPackage(id, dependencies));
+        dependenciesOnly.add(getDepOnlyPackage(id, dependencies));
         idOutputLine.put(id, String.format("%s,%s,%s,,,\n",
             paths.peek(),
             hierarchy.peek(),
-            detectPackageType(vaultPackage)));
+                PACKAGE_TYPE));
     }
 
-   
-
     @Override
-    public void endPackage() {
+    public void endPackage(@NotNull PackageId id, @NotNull VaultPackage vaultPackage) {
+        idOutputLine.computeIfPresent(id, (key, value) -> value.replace(PACKAGE_TYPE, detectPackageType(vaultPackage).toString()));
         paths.pop();
         hierarchy.pop();
     }
 
     @Override
-    public void startSubPackage(@NotNull String path, @NotNull VaultPackage vaultPackage) {
-        PackageId id = vaultPackage.getId();
+    public void startSubPackage(@NotNull String path, @NotNull PackageId id, @NotNull VaultPackage vaultPackage) {
         Dependency[] dependencies = vaultPackage.getDependencies();
         paths.add(path);
         String absolutePath = paths.stream().collect(joining(PATH_SEPARATOR_CHAR));
 
-        packages.add(getDepOnlyPackage(id, dependencies));
+        dependenciesOnly.add(getDepOnlyPackage(id, dependencies));
         idOutputLine.put(vaultPackage.getId(), String.format("%s,%s,%s,%s,%s,%s\n",

Review comment:
       that looks like a leftover.... shouldn't it also be 'id'




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org