BuildKonfig icon indicating copy to clipboard operation
BuildKonfig copied to clipboard

Generating Secondary and more BuildKonfig object

Open mustafaozhan opened this issue 2 years ago • 9 comments

Currently, everything that is generated is added into a single object. That sounds reasonable but we may want to group certain things and seperate from eachother, maybe we can introduce a name into config and use this name to name generated objects

like this

configure<BuildKonfigExtension> {
    packageName = "${ProjectSettings.PROJECT_ID}.common"
    name = "VersionBuildKonfig"

    defaultConfigs {
            buildConfigField(STRING, "VERSION", project.getVersion(), const = true)
    }
}

configure<BuildKonfigExtension> {
    packageName = "${ProjectSettings.PROJECT_ID}.common"
    name = "ApiBuildKonfig"

    defaultConfigs {
            buildConfigField(STRING, "BASE_URL", "www.asd.com", const = true)
    }
}

Additionally, this request will also make sense after we implement: https://github.com/yshrsmz/BuildKonfig/issues/74 Since we may want to make some Konfigs internal while some public

mustafaozhan avatar Nov 22 '22 20:11 mustafaozhan

Can I take on this task?

l2hyunwoo avatar Oct 02 '23 03:10 l2hyunwoo

@l2hyunwoo Sure, but we first need to design & agree on the new API. Do you have anything in mind?

I think SQLDelight is a good source of ideas. https://cashapp.github.io/sqldelight/2.0.0/android_sqlite/

yshrsmz avatar Oct 03 '23 03:10 yshrsmz

@yshrsmz Oh thanks to provide good reference :)

I skimmed the gradle plugin reference, and it is easy to use I think.

It'll be nice that configuration api should be based on above one. And it'll be better with integrating(or renaming) some properties in this task.


configure<BuildKonfigExtension> {
    packageName = "${ProjectSettings.PROJECT_ID}.common"
    name = "ApiBuildKonfig"
    flavor = "dev(stage, release etc)"
    visibility = INTERNAL/PUBLIC
    

    defaultConfigs {
            buildConfigField(STRING, "BASE_URL", "www.asd.com", const = true)
    }
}

l2hyunwoo avatar Oct 04 '23 01:10 l2hyunwoo

Assuming from your snippet you want to define a buildkonfig extension block for each BuildKonfig object, right?

configure<BuildKonfigExtension> {
  name = "ApiBuildKonfig"
  // ...
}

configure<BuildKonfigExtension> {
  name = "SecretBuildKonfig"
  // ...
}

I'm unsure if we can ensure that BuildKonfig object names are distinct with this setup.

yshrsmz avatar Oct 04 '23 07:10 yshrsmz

Assuming from your snippet you want to define a buildkonfig extension block for each BuildKonfig object, right?

right!

I'm unsure if we can ensure that BuildKonfig object names are distinct with this setup.

I understood what you worried about. When I was writing above design, I lost that design can't distinct objects brcause of configuration block is separated.

It'll be resolved with below structure.

defaultConfigs {
    register {
        packageName = "${ProjectSettings.PROJECT_ID}.common"
        name = "ApiBuildKonfig"
        flavor = "dev(stage, release etc)"
        visibility = INTERNAL/PUBLIC

        defaultConfigs {
            buildConfigField(STRING, "BASE_URL", "www.asd.com")
        }
    }
    register {
        packageName = "${ProjectSettings.PROJECT_ID}.core"
        name = "CommonKonfig"
        flavor = "dev(stage, release etc)"
        visibility = INTERNAL/PUBLIC

        defaultConfigs {
            buildConfigField(STRING, "BASE_URL", "www.asdd.com")
        }
    }
}

With above structure, configuration block will transform to NamedDomainObjectContainer , so it can distinct object with name.

Reference - SQLDelightPlugin

l2hyunwoo avatar Oct 05 '23 04:10 l2hyunwoo

You may have misunderstood the current API a bit(or just a typo?).

Here's a minimal configuration block for a simple BuildKonfig(current API)

buildkonfig {
    packageName = "com.example.app"

    // default config is required
    defaultConfigs {
        buildConfigField(STRING, "name", "value")
    }
}

If you want to add some flavored config, you can add those by defining targetConfigs.

buildkonfig {
    packageName = "com.example.app"

    // default config is required
    defaultConfigs {
        buildConfigField(STRING, "name", "value")
    }

    targetConfigs {
        // names in create should be the same as target names you specified
        create("android") {
            buildConfigField(STRING, "name2", "value2")
            buildConfigField(STRING, "nullableField", "NonNull-value", nullable = true)
        }

        create("ios") {
            buildConfigField(STRING, "name", "valueForNative")
        }
    }
}

So, you can't nest defaultConfigs, and can't define flavor next to packageName or objectName like you wrote.

Here's my proposal, inspired by yours. What do you think?

buildkonfig {
  // keep the current API for backward compatibility, if possible
  packageName = "com.example.app"
  defaultConfigs {
    // ...
  }

  // below is the new API

  // register method should receive the *required* name parameter so that we can make sure that everyone names their configs.
  register(name = "AppConfig") {
    packageName = "com.example.app"
    // As the name is a required parameter, we can simply provide an option for visibility
    // see the previous discussion: https://github.com/yshrsmz/BuildKonfig/issues/31
    visibility = INTERNAL/PUBLIC

    defaultConfigs {
      buildConfigField(STRING, "name", "value")
    }

    // I still think it's good idea to pass flavor as a parameter for defaultConfigs/targetConfigs
    defaultConfigs(flavor = "dev") {
      // flavored defaultConfigs
    }

    targetConfigs {
      // default targetConfigs
    }

    targetConfigs(flavor = "dev") {
      // flavored targetConfigs
    }
  }

  register(name = "ApiConfig") {
    packageName = "com.example.app"
  }
}

yshrsmz avatar Oct 08 '23 09:10 yshrsmz

@yshrsmz As you said, it would be good to keep the existing APIs alive for compatibility. Also your new proposal is so reasonable to implement. Thanks for the great suggestion!

l2hyunwoo avatar Oct 09 '23 04:10 l2hyunwoo

@yshrsmz If there's no doubt, I'll start this task, is it okay?

l2hyunwoo avatar Oct 12 '23 03:10 l2hyunwoo

@l2hyunwoo sure, let's do this 💪

yshrsmz avatar Oct 12 '23 03:10 yshrsmz