.xtb auto discovery #172
.xtb auto discovery #172
Conversation
There was a problem hiding this comment.
Okay, there are some surface level comments, but let me suggest a slightly different approach?
First, let's keep the config and the inputs separate - the task should be responsible for its inputs, filtering down to the right one, etc. The task/taskfactory can also be a bit more clever about reading keys - you do need to take some pains to make sure maven knows what is going on, but just a boring bean (or even a map) should be enough for that - and specifically in the maven project (as you did). But, the path to the config is enough for the task factory to follow - no need for a real type in config...
For example, the Config type has the ability to get nested keys - in the taskfactory.resolve method you can call internally get("translationsFile") and then call nested properties, or you can actually call config.getString("translationsFile.auto") to get that actual value, and if not present, call config.getFile("translationsFile.file") to read the file. In turn, you can call config.getString("defines.goog.LOCALE") to read that local (the config system tests if you could have a . in the key, or if it should dig deeper into the structure to find the next node).
At this time, only the ClosureTask will need this filtering feature - after the stage 1/2/3 compile is introduced it might be necessary to use it in more than one place, but we can refactor that out as needed (i.e. this isn't your problem).
Your per-run logic of re-filtering the contents of the inputs is correct, that should correctly support j2cl:watch noticing a new .xtb file on the fly, likewise your filtering to include only scope=runtime and type=BYTECODE contents.
--
Off the top of my head, I can't think of a reason to require a particular path for the xtb files - java projects tend to put message bundles in the package structure, do you know where closure projects tend to keep their xtb files?
| private File file; | ||
|
|
||
| private Config config; | ||
| List<com.vertispan.j2cl.build.task.Input> jsSources; |
There was a problem hiding this comment.
Please split this file, so that inputs (part of tasks) don't end up in configs - instead make a new type in j2cl-tasks, maybe something that wraps this type? Probably not necessary to actually wrap this though, based on my understanding of this feature...
| throw new RuntimeException(e); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (SAXException e) { |
There was a problem hiding this comment.
coalesce these, maybe add a message to be clearer about which file it was (like the runtime ex above does)?
|
|
||
| String lang = translationbundleNode.item(0).getAttributes().getNamedItem("lang").getNodeValue(); | ||
|
|
||
| if(locale.equals(lang)) { |
There was a problem hiding this comment.
do you need to handle substrings, i.e. locale vs language, as in en_us vs en_ca?
There was a problem hiding this comment.
gitter discussion covered deferring this locale naming work: please file a vertispan/j2clmavenplugin issue, and reference that issue by number in the code, something like
//TODO vertispan/j2clmavenplugin#XYZ update to use full locale names, not just language string
so that anyone confused by this behavior looking at the code knows where to find followup, what ticket to update.
my relatively recent experience in github issues is that the org/project should be included in the reference, so that if the project is forked and maintained elsewhere, the reference still makes sense (since forks dont keep issues)s
| private Config config; | ||
| List<com.vertispan.j2cl.build.task.Input> jsSources; | ||
|
|
||
| public TranslationsFileConfiguration(Config config, File file) { |
There was a problem hiding this comment.
in this ctor, auto is effectively false, right? maybe make file and auto final, assign them both? or make file an optional, so that there is just one field to hold that?
| @@ -138,7 +139,7 @@ public class BuildMojo extends AbstractBuildMojo { | |||
| * Closure flag: "Source of translated messages. Currently only supports XTB." | |||
| */ | |||
| @Parameter | |||
| protected String translationsFile; | |||
| protected TranslationsFileConfig translationsFile; | |||
There was a problem hiding this comment.
Ah got it. you use the two fields so that it makes sense to maven. What about just making this a Map<String, String> so that maven doesn't care what is there, or a dummy type to show it the "schema", but let the config read it out how it wants to?
treblereel
force-pushed
the
xtb_auto_discovery
branch
from
3592365
to
5728e6b
Compare
| Optional<File> translationsfile = config.getTranslationsFile(); | ||
|
|
||
| TranslationsFileProcessor translationsFileProcessor = new TranslationsFileProcessor(config); | ||
| translationsFileProcessor.setProjectInputs( |
There was a problem hiding this comment.
i don't like it, but looks like it's the only place i can pass .xtb to processor
@niloc132 could we do better ?
|
@niloc132 may i ask you to rerun tests ? |
this PR bring ability to automatically provide proper (.xtb that contains translations bundle for the current locale) .xtb to closure.
For instance, we could generate .xtb bundles using apt like in treblereel/gwt3-processors#17