yshrsmz / BuildKonfig

BuildConfig for Kotlin Multiplatform Project
Apache License 2.0
739 stars 33 forks source link

Generating Secondary and more BuildKonfig object #75

Open mustafaozhan opened 1 year ago

mustafaozhan commented 1 year ago

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

l2hyunwoo commented 11 months ago

Can I take on this task?

yshrsmz commented 11 months ago

@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/

l2hyunwoo commented 11 months ago

@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)
    }
}
yshrsmz commented 11 months ago

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.

l2hyunwoo commented 11 months ago

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

yshrsmz commented 11 months ago

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"
  }
}
l2hyunwoo commented 11 months ago

@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 commented 11 months ago

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

yshrsmz commented 11 months ago

@l2hyunwoo sure, let's do this 💪