gmazzo/gradle-buildconfig-plugin

Any reason not to use a `LinkedHashMap` as before?

jasonsparc opened this issue · 3 comments

Recently, I updated to 4.0.4 and apparently I wasn't aware that the ordering of the fields are no longer being taken into account. The previous implementation used to use a LinkedHashMap to collect the fields, and I took advantage of that fact and have my fields depend on each other.

Now sadly, the current implementation now uses a NamedDomainObjectContainer.

As far as I know, you could use a Map for @Nested (according to its API docs), and thus, you should be able to use a LinkedHashMap with it. Now I'm not sure how it would behave if the configuration cache is enabled, but let's just assume that, that would be a Gradle bug instead; since after all, the configuration cache should be able to restore objects back to their original types (or throw an error otherwise if it can't).

Not a particular reason, just felt more correct to use it. I think I didn't consider the use case of making constants depending on others.

I could use another implementation that keeps the insertion order, but I don't want to add another backward incompatible change.

I think a what I can do is to add an order parameter that will automatically increment on each add. WDYT?

I think a what I can do is to add an order parameter that will automatically increment on each add. WDYT?

I think that's an acceptable compromise.

Looking at the PR #58, I think I'm fine with that – as a bonus, it's also less dependent on Gradle behavior (unlike the LinkedHashMap implementation that I mentioned).

Maybe also add a test that does something similar to the following:

buildConfig {
    buildConfigField("String", "FIRST", "1")
    buildConfigField("String", "SECOND", "FIRST + 1")
    buildConfigField("String", "THIRD", "SECOND + 1")
}

Asserting that FIRST == 1 && SECOND == 2 && THIRD == 3 upon generating the build config constants.

Thanks for reporting this. I've added a test and it will be released as 4.1.0