yshrsmz / BuildKonfig

BuildConfig for Kotlin Multiplatform Project
Apache License 2.0
782 stars 32 forks source link

Allow BuildKonfig to be public #31

Closed dalewking closed 4 years ago

dalewking commented 4 years ago

Object is generated as internal. To use it from the iOS or Android app requires adding another layer to expose the values. Add an option to generate it as public.

yshrsmz commented 4 years ago

This is intended behavior, as the object is named BuildKonfig(which is obviously not the name people intentionally named as their API) and I think it should not be exposed as a part of public API.

This option should be paired with an option to change the name of the generated object.

dalewking commented 4 years ago

As I said it should be optional and default to current behavior.

yshrsmz commented 4 years ago

I understand, and I think we first need to add an option to change the name of the generated object.

romanandreyvich commented 4 years ago

Any progress here?

yshrsmz commented 4 years ago

Nothing really

PR is really welcome

yshrsmz commented 4 years ago

The configurable object name is supported since v0.6.0. So if anyone wants to add an option to make generated objects public, you are good to go.

yshrsmz commented 4 years ago

How should we configure this option?

buildkonfig {
    packageName = 'com.example.app'
    objectName = 'AwesomeConfig'
    exposeObject = true

    defaultConfigs {
        buildConfigField 'STRING', 'name', 'value'
    }
}

Above is one option, but I think we should avoid this as we cannot infer the requirement.

So basically I've been thinking about these two options below.

buildkonfig {
    packageName = 'com.example.app'
    exposeObjectWithName = 'AwesomeConfig'

    defaultConfigs {
        buildConfigField 'STRING', 'name', 'value'
    }
}

With this, we can be explicit about the requirement, but now we have multiple ways to configure the object name.

Another option is to make objectName property a function and add a second parameter.

fun objectName(name: String, expose: Boolean = false)

buildkonfig {
    packageName = 'com.example.app'
    objectName('AwesomeConfig', true)

    defaultConfigs {
        buildConfigField 'STRING', 'name', 'value'
    }
}

This is less explicit, but no duplication.

🤔

Any idea?

dalewking commented 4 years ago

Doesn't make a lot of difference. I was expecting a boolean parameter that controlled whether it was public or internal

dalewking commented 4 years ago

Don't understand what you meant by "we cannot infer the requirement" unless you were saying you have to rename it to expose it. If so I see no reason why that is a requirement. Whether you want to change the name or you want it to be public are really unrelated. I have no problem with exposing it as BuildKonfig so I favor the first choice.

yshrsmz commented 4 years ago

Thanks for your input! I really appreciate it.

As I said before I don't want BuildKonfig to be a part of someone's public API.

Say I have an api module that wants to expose server URL using BuildKonfig and the presentation module wants to use it. What api module should expose is ApiConfig or Endpoint object, but shouldn't be BuildKonfig. BuildKonfig tells nothing about its content and I think it's not a good design decision to expose it as is. So I won't make that happen in the first place.

I'm leaning towards the second one.

buildkonfig {
    packageName = 'com.example.app'
    exposeObjectWithName = 'AwesomeConfig'

    defaultConfigs {
        buildConfigField 'STRING', 'name', 'value'
    }
}
yshrsmz commented 4 years ago

Though I must agree that the first one looks more concise. Maybe we could go with the first one and throw the exception when people forget to change the name.

dalewking commented 4 years ago

I agree that naming it BuildKonfig in a public library is probably not what you want, but in our case it is just exposing values for the Android and iOS apps to use so I don't care what it is called.

If someone wants to make it public and leave it named BuildKonfig, why do you care? You allow them to change it, not sure why the need to force them to change it.

On Wed, Aug 19, 2020 at 12:02 PM Yasuhiro SHIMIZU notifications@github.com wrote:

Though I must agree that the first one looks more concise. Maybe we could go with the first one and throw the exception when people forget to change the name.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/yshrsmz/BuildKonfig/issues/31#issuecomment-676515281, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACUPTQILLSAXITBQ24ZCG3SBPZRDANCNFSM4MM3NZBQ .

-- Dale King