Tools module and BundleLineCreator for #14

[cypherdare]

This adds a class for generating BundleLines mostly automatically. It can be used from an app or the module can be imported as a buildscript dependency and used as a gradle plugin that adds a task for generating the BundleLines. See the README for a complete description.

How rigorous should the testing be? I'm not sure how to appropriately test some of the behaviors like finding the assets directory. Unlike other Ktx utilities, this tool is more like a development script, not a component relied on by game code.

Feel free to edit this branch. I don't know if I've appropriately set up the new module.

[czyzby]

Thanks for this contribution! I originally wanted to make a Gradle plugin with some KTX utilities, but never went far. See #14.

How rigorous should the testing be?

Testing the generated output (as a string) would be a good start. I agree that we don't need as rigorous testing as in case of the libraries, but I'd still like the code to be verified automatically.

I'm not sure how to appropriately test some of the behaviors like finding the assets directory.

If difficult to setup otherwise, write a function that searches for properties files in any set of directories (e.g. temp folders in case of tests) and assume it works correctly given the LibGDX default asset directories.

I don't know if I've appropriately set up the new module.

I believe it's OK. You can go through this contribution section to see if you missed anything. Also, make sure to format the code with a Gradle task, as it breaks the CI check.

---

Just a heads up - I'm going on a 1 week vacation without internet access. I'll review the PR after I'm back.

[cypherdare]

OK. I've added some tests. One piece I don't have a practical test for is checking the validity of the generated code itself.

[cypherdare]

I also refactored a bit to better encapsulate the tool and set a pattern for implementing additional tools in the future that can both be used as Gradle tasks or from a JVM app. Here's an explanation of the setup.

In the spirit of many LibGDX classes, the BundleLinesCreator class is open, and most of its methods are open. This allows someone to subclass it and modify some of the behaviors, such as how it selects which files to process. But it has a companion object with the default behavior, so it can be used directly with BundleLinesCreator.execute(). Following the precedent from the stdlib Random class, the companion object is named Default.

The execute function also has an overload where you can pass a single parameters object. This is internal because it's only intended for the Gradle plugin to use. The Plugin only has to call BundleLinesCreator.execture() and pass it the parameters from the KtxToolsPluginExtension object. The parameters object helps organize settings in the plugin extension.

So, for any future utility classes, another parameters object property can be added for it in KtxToolsPluginExtension. This will allow all the task parameters to be set in one block:

ktxtools {
    createBundleLines.targetPackage = "com.game"
    someOtherTask.option = true
}

[cypherdare]

I added a test to check the actual produced code. It's not an exhaustive test for everything that could matter as that would be a mammoth task to undertake. It uses regex to extract various elements that we can check easily. There are pieces that aren't useful to check because it would amount to copy-pasting pieces of code we want to generate into the test. For the pieces of code that are checked, the test ignores formatting and syntax variations as long as they're valid (such as type of whitespace, optional newlines, and optional whitespace).

The generated code matches what the default Kotlin formatter in IntelliJ does, as this is the most likely desired formatting.

[czyzby]

There are pieces that aren't useful to check because it would amount to copy-pasting pieces of code we want to generate into the test.

That's actually not a bad way to test this, in my opinion. After all, in typical tests you'd ideally like to end up with a single equality check of the produced output, not a bunch partial checks verifying only parts of the result. I think it's reasonable to copy or write the expected code in a multiline string and compare it against the generated class, as it would be a better indicator of valid code generation than e.g. matching closing brackets - and it's easy to add extra tests for settings like custom class name or indent.

Thanks for the contribution, looking good so far.

[cypherdare]

OK, I got everything updated and resolved. I've tested adding it as a buildscript dependency and running the task, and it runs fine.

But when I add it as a dependency and try to run the BundleLinesCreator in an application, it now fails because the Gradle API is missing as a dependency. This is because the BundleLinesCreator class now references the Gradle API Logger class.

I think maybe this is because I didn't make the API a dependency in build.gradle, but rather used the java-gradle-plugin, which automatically adds it as a dependency in the project when I'm working on it. I'm not sure what the distinction is, but I guess by default it doesn't make the API a dependency because it's intended for making plugins, where the Gradle API will obviously always be available.

I haven't looked yet if there is a Gradle API Maven repository I can simply add as an implementation dependency in the build.gradle file so this will work properly.

Got kind of burned out on this the past couple days. I will probably take a week or two before I get back to it, but you're welcome to try solving the above problem in the meantime.

[czyzby]

But when I add it as a dependency and try to run the BundleLinesCreator in an application, it now fails because the Gradle API is missing as a dependency. This is because the BundleLinesCreator class now references the Gradle API Logger class.

Have you tried using SLF4J or any other logger wrapper? I don't think we should spend too much time on this, SLF4J/Log4J do not work with Gradle, we can revert to prints.

Got kind of burned out on this the past couple days. I will probably take a week or two before I get back to it, but you're welcome to try solving the above problem in the meantime.

Understandable. Thanks again for the contribution. I'll probably wait for you to come back, but I'll review the changes in the meantime.

[cypherdare]

I just switched to use SLF4J and it works fine now. One possible issue is that Gradle's logger defaults to "Lifecycle" level which is between SLF4J's "Info" and "Warn". I log the results of the file creation at the info level, so this message won't show when running the task without modifying the log level. This is probably fine, because you don't need this kind of info unless you're trying to debug why the task isn't giving you your expected output.

[cypherdare]

Are the tests failing because SLF4J emits a warning about duplicate bindings? I don't know the proper way to resolve this.

[czyzby]

This is probably fine, because you don't need this kind of info unless you're trying to debug why the task isn't giving you your expected output.

That's the default behavior of Gradle. That's OK.

Are the tests failing because SLF4J emits a warning about duplicate bindings? I don't know the proper way to resolve this.

I don't think so, I believe it's just a warning. If you dig deeper that's the main cause:

Execution failed for task ':tools:lint'.

My guess is you just have to reformat the sources with gradle format.

[cypherdare]

Haha, the imports were not alphabetized!

[czyzby]

Thanks @cypherdare, looks good now. Could you update to the latest develop changes with LibGDX 1.9.11 and add a changelog entry briefly describing the new module features?

[deveth0]

@cypherdare awesome work, thank you so much! I tried to integrate the code into a small sample project and ran into some issues regarding the enum names.
Look at the following file foo.properties:

foo=bar
foo.lorem=ipsum

This will generate this enum:

@Suppress("EnumEntryName")
enum class Menu : BundleLine {
    foo,
    `foo.lorem`;
...

Sadly this is not valid code. What do you think about changing the generation so that it generates something like this;

@Suppress("EnumEntryName")
enum class Menu : BundleLine {
    FOO("foo"),
    FOO_LOREM("foo.lorem");
...

[czyzby]

That's actually a good point, we should add a line like that to the test suite.

@deveth0, what would you say about creating a nested enum class, so that the generated code closely resembles the properties file structure? For example:

enum class Menu : BundleLine {
  // ...

  enum class foo {
    forem
  }
}

fun usageExample() {
  Menu.foo.forem
}

@cypherdare I'm sorry about dragging this PR for so long, I'll try to merge it this month. If you don't have more time to work on this, I'll change the generation code to support nested enums or better escaping.

[deveth0]

Hm, we had a similar discussion in another context and the general agreement was, that property files don't really support nesting and this is a weird hack ;)
I also think that this might result in a real clutter if the "nesting" becomes too deep.

Actually I was a bit bored and played around with the second part of the implementation, the generation of Asset Atlas enums. You might run into a similar problem there cause the name of the Region is not necessarily a valid Enum name. So I think the safest way is to keep the original String available and use a safe version as identifier.

Please have a look into this repository:
https://github.com/deveth0/ktx-tools-demo

I stripped down @cypherdare 's code a bit and extracted a generator for Enums using kotlinpoet. The demo-project automatically runs the two tasks (createBundleLines and createAssetEnums) and places the generated sources in /build/generated-sources. You can find the plugins in /buildSrc, that was the easiest approach for now ;)

Imho there is no real reason why those files should not be generated during the build, this also makes sure, that they are always up to date. As they are automatically generated code, I'd also suggest to exclude them from ktlint.

[czyzby]

Makes sense, it's similar to how other Java projects work. I pretty sure you can also automatically mark folders with the generated files as source folders when applying the plugin. Kotlinpoet is a nice touch. This is actually a very nice expansion of the original plugin - would you like to submit your code as a PR? I could setup a separate branch for ktx-tools with @cypherdare's changes that you can fork.

[cypherdare]

Good catch.

I don't have time in the very near future to add nesting, so if either of you wants to flesh this out more, that's fine with me.

In my opinion, it should not automatically change property names into SNAKE_CASE. I personally prefer PascalCase for enum constants. As it is, you can control the case type yourself to your preference by your choice in the properties file.

I originally thought about using KotlinPoet, but thought it was overkill originally. But it isn't if you're adding nesting.

What is the reason to break out the EnumCreator abstract class?

[czyzby]

I agree about preserving the case. While it might require adjustments in case of the texture atlases, I think it's the most flexible approach for i18n bundles - we're simply letting the users decide how their identifies look.

As for the nesting - I don't necessarily see this as a hack, as it allows to preserve the user-defined structures within the properties files (even if somewhat artificial). It also has the additional benefits of supporting selective imports - e.g. you can star-import only specific bundle lines assigned to a chosen screen, or use code completion to find a specific line in a specific enum. I could see that as better than just generating a single enormous enum and basically forcing the users to split the bundle into multiple files to achieve a similar (but less convenient) effect.

The only problem that I can see is the issue with name collisions. For example, given foo and foo.bar properties, you cannot both add a foo bundle line and a foo enum class to support nesting. To fix that, I'd add a plugin setting that allows the user to either issue a warning or throw an error during the generation when encountering name collisions.

And by the way, changing the properties' names to uppercase wouldn't entirely solve the problem of name collisions either. For example, if we replaced special characters with underscores, both foo.bar and foo-bar would end with the same FOO_BAR identifier. It also loses the benefit of grouping chosen properties in code. If both approaches end up with different issues, I'd rather go with the one that's more flexible and the one that generates code closer to the original users' configuration.

[deveth0]

Thank you both for your feedback, really appreciate that!
@cypherdare I needed a plugin for my AssetAtlasEnum and as a lazy developer, I was looking for some inspiration and found your code. So the natural choice was to analyze, which parts I could reuse and this resulted in the EnumCreator.
This was also the main reason why I replaced the static generation with kotlinpoet, it's more flexible than manipulating strings in subclasses.

So both of you agree, that the enum type names should keep the original names of i18n keys and use nesting? Fine with that, the * import is a rather good argument!

@czyzby as I have virtually NO clue, how to use a gradle plugin from another project, I'd prefer to keep the code in the demo project I setup and play around a bit with nesting etc until we come to a state where we all agree, that this is the way to go. I'd then take the ktx-tools branch and integrate my changes.

[czyzby]

@deveth0 Sure, take your time.

[deveth0]

Looking at the code again: Is there really a reason, why multiple files should be merged into the same Enum?
This brings in some potential problems we can easily avoid by removing this option. Mainly duplicates become hard to detect as the second entry just overrides the first one. Also it's no longer possible to track the origin of the entry.

To be honest I never used the i18n feature in libgdx so I might just be ignorant here ;)

[czyzby]

If the .properties files have the same core name (as in, they are different variants/languages of the same properties set), we should gather all properties and put them in the same enum. It's essentially the same translation bundle.

As for multiple properties files with different names, I think generating separate enums might be even easier to manage for the users code-wise, as you might need to load them with separate I18NBundle instances either way.

[czyzby]

If you want to work on this, I think it might be a good idea to give ktx-i18n a try just to see how it works.

[deveth0]

@czyzby I definitly will ;) Merging all values into a single enum is a feature @cypherdare has implemented, I'm just not sure, if this is required.

[czyzby]

OK, so to clear this up - given these files:

• MyBundle.properties
• MyBundle_en.properties
• MyBundle_fr.properties
• OtherBundle.properties

The plugin should generate two enums: MyBundle with properties from the first three files with the same core name and OtherBundle with properties from the last file.

EDIT: To make it a bit easier, we can assume that anything before the first underscore is the core name of the bundle. We can document these assumptions.

[deveth0]

Exactly :) I was refering to https://github.com/cypherdare/ktx/blob/tools/tools/src/main/kotlin/ktx/tools/BundleLinesCreator.kt#L259

[czyzby]

Right, so this is something we can change before merging the ktx-tools into master. I think it makes more sense to separate different bundles into multiple files, even from the practical point of view - the users might want or need to maintain multiple separate bundles, and putting all properties in a single enum only makes it more error-prone. The plugin property that you highlighted can be removed and the class name can be extracted from the file name. Let's assume that anything before the first underscore is the core name of the bundle.

[czyzby]

I'm going to close this PR for now, as the plugin currently does not generate multiple separate bundles correctly. If either of you would like to work on this one @cypherdare @deveth0, feel free to set up another PR. Thanks.