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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
