Code review feedback in a patch

build-caching/src/main/java/com/vertispan/j2cl/build/PropertyTrackingConfig.java

@@ -141,22 +141,6 @@ public Collection<String> getExterns() {
         return Collections.emptySet();
     }
 
-    @Override
-    public Map<String, Object> getTranslationsFile() {
-        ConfigValueProvider.ConfigNode translationsFile = config.findNode("translationsFile");
-        if (translationsFile == null) {
-            return Collections.emptyMap();
-        }
-        return translationsFile.getChildren().stream()
-                .collect(Collectors.toMap(ConfigValueProvider.ConfigNode::getName, e -> {
-                    if(e.getName().equals("file")) {
-                        return e.readFile();
-                    } else {
-                        return e.readString();
-                    }
-                }));
-    }
-
     @Override
     public boolean getCheckAssertions() {
         return Boolean.parseBoolean(getString("checkAssertions"));

build-caching/src/main/java/com/vertispan/j2cl/build/task/Config.java

@@ -1,6 +1,5 @@
 package com.vertispan.j2cl.build.task;
 
-
 import java.io.File;
 import java.nio.file.Path;
 import java.util.Collection;
@@ -24,8 +23,6 @@ public interface Config {
 
     Collection<String> getExterns();
 
-    Map<String, Object> getTranslationsFile();
-
     boolean getCheckAssertions();
 
     boolean getRewritePolyfills();

j2cl-maven-plugin/src/main/java/com/vertispan/j2cl/mojo/Xpp3DomConfigValueProvider.java

@@ -205,7 +205,13 @@ private Xpp3Dom findNodeWithKey(Xpp3Dom config, String prefix, String remaining)
             if (index == -1) {
                 return null;// failed to find it so far, must not be present
             }
-            return findNodeWithKey(config, prefix.substring(0, index), prefix.substring(index + 1) + '.' + remaining);
+            String nextRemaining;
+            if (remaining.isEmpty()) {
+                nextRemaining = prefix.substring(index + 1);
+            } else {
+                nextRemaining = prefix.substring(index + 1) + '.' + remaining;
+            }
+            return findNodeWithKey(config, prefix.substring(0, index), nextRemaining);
         }
     }
 

j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/ClosureBundleTask.java

@@ -18,6 +18,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -100,7 +101,7 @@ public Task resolve(Project project, Config config) {
                     Collections.emptyList(),
                     Collections.emptyMap(),
                     Collections.emptyList(),//TODO actually pass these in when we can restrict and cache them sanely
-                    new TranslationsFileProcessor(config),
+                    Optional.empty(),
                     true,//TODO have this be passed in,
                     true,//default to true, will have no effect anyway
                     false,

j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/ClosureTask.java

@@ -179,18 +179,15 @@ public Task resolve(Project project, Config config) {
         //TODO probably kill this, or at least make it work like an import via another task so we detect changes
         Collection<String> externs = config.getExterns();
 
-        TranslationsFileProcessor translationsFileProcessor = new TranslationsFileProcessor(config);
-        translationsFileProcessor.setProjectInputs(
-                Stream.concat(
-                                Stream.of(project),
-                                scope(project.getDependencies(), com.vertispan.j2cl.build.task.Dependency.Scope.RUNTIME).stream()
-                        )
-                        .map(p ->
-                                input(p, OutputTypes.BYTECODE)
-                        )
-                        // Only include the .xtb
-                        .map(i -> i.filter(XTB))
-                        .collect(Collectors.toList()));
+        TranslationsFileProcessor translationsFileProcessor = TranslationsFileProcessor.get(config);
+        List<Input> xtbInputs = Stream.concat(
+                        Stream.of(project),
+                        scope(project.getDependencies(), Dependency.Scope.RUNTIME).stream()
+                )
+                .map(p -> input(p, OutputTypes.BYTECODE))
+                // Only include the .xtb
+                .map(i -> i.filter(XTB))
+                .collect(Collectors.toList());
 
         boolean checkAssertions = config.getCheckAssertions();
         boolean rewritePolyfills = config.getRewritePolyfills();
@@ -255,7 +252,7 @@ public void execute(TaskContext context) throws Exception {
                         entrypoint,
                         defines,
                         externs,
-                        translationsFileProcessor,
+                        translationsFileProcessor.getTranslationsFile(xtbInputs, context.log()),
                         true,//TODO have this be passed in,
                         checkAssertions,
                         rewritePolyfills,

j2cl-tasks/src/main/java/com/vertispan/j2cl/build/provided/TranslationsFileProcessor.java

@@ -1,6 +1,9 @@
 package com.vertispan.j2cl.build.provided;
 
-import com.vertispan.j2cl.build.task.*;
+import com.vertispan.j2cl.build.task.BuildLog;
+import com.vertispan.j2cl.build.task.CachedPath;
+import com.vertispan.j2cl.build.task.Config;
+import com.vertispan.j2cl.build.task.Input;
 import org.w3c.dom.Document;
 import org.w3c.dom.NodeList;
 import org.xml.sax.SAXException;
@@ -16,80 +19,61 @@
 import java.util.Optional;
 import java.util.stream.Collectors;
 
-public class TranslationsFileProcessor {
+public interface TranslationsFileProcessor {
+    public static TranslationsFileProcessor get(Config config) {
+        File file = config.getFile("translationsFile.file");
+        boolean auto = Boolean.parseBoolean(config.getString("translationsFile.auto"));
 
-    private final Config config;
-
-    private XTBLookup lookup = new TranslationsFileNotDefined();
-
-    private List<Input> jsSources;
-
-    public TranslationsFileProcessor(Config config) {
-        this.config = config;
-        if (!config.getTranslationsFile().isEmpty()) {
-            if (!config.getCompilationLevel().equals("ADVANCED")) {
-                //Do we have logger ?
-                System.out.println("translationsFile only works in the ADVANCED optimization level, in other levels the default messages values will be used");
-            } else {
-                if (config.getTranslationsFile().containsKey("file")) {
-                    //translation file explicitly defined
-                    lookup = new ExplicitlyDefined();
-                } else if (config.getTranslationsFile().containsKey("auto") && config.getTranslationsFile().get("auto").equals("true")) {
-                    // we have to perform Project wide lookup
-                    lookup = new ProjectLookup();
-                }
-            }
+        if ((auto || file != null) && !config.getCompilationLevel().equals("ADVANCED")) {
+            return new TranslationsFileNotDefined(true);
         }
+        if (file != null) {
+            return (inputs, log) -> Optional.of(file);
+        } else if (auto) {
+            return new ProjectLookup(config);
+        }
+        return new TranslationsFileNotDefined(false);
     }
 
-    public Optional<File> getTranslationsFile() {
-        return lookup.getFile();
-    }
-
-    public void setProjectInputs(List<Input> collect) {
-        this.jsSources = collect;
-    }
-
-    private interface XTBLookup {
+    Optional<File> getTranslationsFile(List<Input> inputs, BuildLog log);
 
-        Optional<File> getFile();
-    }
+    class TranslationsFileNotDefined implements TranslationsFileProcessor {
+        private final boolean shouldWarn;
 
-    private class TranslationsFileNotDefined implements XTBLookup {
+        public TranslationsFileNotDefined(boolean shouldWarn) {
+            this.shouldWarn = shouldWarn;
+        }
 
         @Override
-        public Optional<File> getFile() {
+        public Optional<File> getTranslationsFile(List<Input> inputs, BuildLog log) {
+            if (shouldWarn) {
+                log.warn("translationsFile only works in the ADVANCED optimization level, in other levels the default messages values will be used");
+            }
+
             return Optional.empty();
         }
     }
 
-    private class ExplicitlyDefined implements XTBLookup {
-
-        @Override
-        public Optional<File> getFile() {
-            System.out.println("getFile " + config.getTranslationsFile().get("file"));
+    class ProjectLookup implements TranslationsFileProcessor {
+        private final String locale;
 
-            return Optional.of((File) config.getTranslationsFile().get("file"));
+        public ProjectLookup(Config config) {
+            locale = config.getString("defines.goog.LOCALE");
         }
-    }
-
-    private class ProjectLookup implements XTBLookup {
 
         @Override
-        public Optional<File> getFile() {
-            List<File> temp = jsSources.stream()
+        public Optional<File> getTranslationsFile(List<Input> inputs, BuildLog log) {
+            List<File> temp = inputs.stream()
                     .map(Input::getFilesAndHashes)
                     .flatMap(Collection::stream)
                     .map(CachedPath::getAbsolutePath)
                     .map(Path::toFile)
                     .collect(Collectors.toList());
 
             if (temp.isEmpty()) {
-                System.out.println("no .xtb files was found");
+                log.warn("no .xtb files was found");
             }
 
-            String locale = config.getDefines().get("goog.LOCALE");
-
             try {
                 DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
 
@@ -117,13 +101,11 @@ public Optional<File> getFile() {
                         return Optional.of(xtb);
                     }
                 }
-            } catch (ParserConfigurationException e) {
-                throw new RuntimeException(e);
-            } catch (IOException e) {
-                throw new RuntimeException(e);
-            } catch (SAXException e) {
+            } catch (ParserConfigurationException | SAXException | IOException e) {
+                log.error("Error while reading xtb files ", e);
                 throw new RuntimeException(e);
             }
+            log.warn("No matching locales for " + locale);
             return Optional.empty();
         }
 

j2cl-tasks/src/main/java/com/vertispan/j2cl/tools/Closure.java

@@ -3,7 +3,6 @@
 import com.google.javascript.jscomp.*;
 import com.google.javascript.jscomp.Compiler;
 import com.vertispan.j2cl.build.DiskCache;
-import com.vertispan.j2cl.build.provided.TranslationsFileProcessor;
 import com.vertispan.j2cl.build.task.BuildLog;
 import com.vertispan.j2cl.build.task.Input;
 
@@ -16,6 +15,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 public class Closure {
@@ -53,7 +53,7 @@ public boolean compile(
             List<String> entrypoints,
             Map<String, String> defines,
             Collection<String> externFiles,
-            TranslationsFileProcessor translationsFile,
+            Optional<File> translationsFile,
             boolean exportTestFunctions,
             boolean checkAssertions,
             boolean rewritePolyfills,
@@ -106,7 +106,7 @@ public boolean compile(
             jscompArgs.add(extern);
         }
 
-        translationsFile.getTranslationsFile().ifPresent(file -> {
+        translationsFile.ifPresent(file -> {
             jscompArgs.add("--translations_file");
             jscompArgs.add(file.getAbsolutePath());
         });