misc: add visibility build setting
#951
Conversation
lauzadis
changed the title
internal visibilityvisibility build setting
| @@ -160,6 +161,7 @@ data class BuildSettings( | |||
| val generateDefaultBuildFiles: Boolean = true, | |||
| val optInAnnotations: List<String>? = null, | |||
| val generateMultiplatformProject: Boolean = false, | |||
| val visibility: VisibilitySettings = VisibilitySettings.Default, | |||
There was a problem hiding this comment.
These aren't really "build" settings since this type doc comments explicitly talk about gradle build settings. I think maybe we want a new type like "API" settings?
| ) { | ||
| companion object { | ||
| const val SERVICE_CLIENT = "serviceClient" | ||
| const val STRUCTURE = "structure" |
There was a problem hiding this comment.
correctness
I'm not sure we want to model visibility of structure and error settings separate from the service client. If you set structure or error visibility to internal then the service client isn't going to compile anyway. Do we validate the combination of settings? What other use cases are we thinking about?
From where I'm sitting at the moment I think we can get away with the same visibility for all of these (which is either public (default) or internal because private doesn't make sense and will also not compile). I suggest we collapse these to just visibility and make them all the same from that (also validate fromNode settings in tests).
There was a problem hiding this comment.
Yeah this is one thing I expected discussion about. I started with SERVICE_CLIENT and then decided to add STRUCTURE/ERROR to cover the remaining codegenned portions, but I do think collapsing them into one config option makes sense.
There was a problem hiding this comment.
I recommend using an enum too instead of a string.
| ) { | ||
| companion object { | ||
| const val SERVICE_CLIENT = "serviceClient" | ||
| const val STRUCTURE = "structure" |
There was a problem hiding this comment.
I recommend using an enum too instead of a string.
| writer.write("public const val ServiceId: String = #S", ctx.settings.sdkId) | ||
| writer.write("public const val SdkVersion: String = #S", ctx.settings.pkg.version) | ||
| writer.write("$visibility const val ServiceId: String = #S", ctx.settings.sdkId) | ||
| writer.write("$visibility const val SdkVersion: String = #S", ctx.settings.pkg.version) |
There was a problem hiding this comment.
Nit: Mix templating styles makes codegen harder to read. I recommend using #L instead of $visibility. Applies to other changes in this PR.
| enum class Visibility(val value: String) { | ||
| PUBLIC("public"), | ||
| INTERNAL("internal"), | ||
| ; | ||
|
|
||
| companion object { | ||
| public fun fromValue(value: String): Visibility = when (value.lowercase()) { | ||
| "internal" -> INTERNAL | ||
| else -> PUBLIC | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: Consider overriding toString to return value so that you don't have to call .value so many places in codegen.
| public fun fromValue(value: String): Visibility = when (value.lowercase()) { | ||
| "internal" -> INTERNAL | ||
| else -> PUBLIC | ||
| } |
There was a problem hiding this comment.
Correctness: This means silently ignoring the caller's intent when we don't understand it, leading to typos like "itnernal" being treated as PUBLIC. I think we should throw an exception if we don't understand the input value.
|
SonarCloud Quality Gate failed.
|
This PR updates build settings to allow configuring visibility for
serviceClient,structure, anderror. This corresponds to visibility on the generated service client, Smithy Structure shapes, and Smithy Error shapes, respectively.Issue #
N/A
Description of changes
This change is required so we can generate internal-only SDK clients with the proper
internalvisibility, so they don't show up in the public API footprint.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.