Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generating Secondary and more BuildKonfig object #75

Open
mustafaozhan opened this issue Nov 22, 2022 · 9 comments
Open

Generating Secondary and more BuildKonfig object #75

mustafaozhan opened this issue Nov 22, 2022 · 9 comments
Labels
contribution welcome enhancement New feature or request help wanted Extra attention is needed

Comments

@mustafaozhan
Copy link

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: #74 Since we may want to make some Konfigs internal while some public

@yshrsmz yshrsmz added help wanted Extra attention is needed enhancement New feature or request contribution welcome labels Mar 13, 2023
@l2hyunwoo
Copy link

l2hyunwoo commented Oct 2, 2023

Can I take on this task?

@yshrsmz
Copy link
Owner

yshrsmz commented Oct 3, 2023

@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
Copy link

@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
Copy link
Owner

yshrsmz commented Oct 4, 2023

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
Copy link

l2hyunwoo commented Oct 5, 2023

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
Copy link
Owner

yshrsmz commented Oct 8, 2023

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
Copy link

@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
Copy link

l2hyunwoo commented Oct 12, 2023

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

@yshrsmz
Copy link
Owner

yshrsmz commented Oct 12, 2023

@l2hyunwoo sure, let's do this 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants