All classes that have builders should lock down their constructors.

[nanodeath]

Right now classes like AppConfig and ShortcutsConfig have Builder classes that should be used exclusively for constructing instances of those classes, but this isn't guaranteed because the constructors are public, so users could actually use either. Some functionality, like that introduced in #381, assumes users will be using the Builders. Plus it'd be nice to have one Right Way to do things.

Potentially every class with a builder in our Builders package needs to be examined, but the builders themselves also need to be examined. AppConfigBuilder, for example, itself has a public constructor.

The end goal here is for there to be one way to build these things, for simplicity for the builder and to make things easier for the Zircon maintainers. Every public API class in Zircon should conform to one of the following patterns:

1. It has a public constructor and no builder, or
2. It has an internal constructor, a corresponding builder class (which itself has an internal constructor), and the way you construct the instance is by calling a public TheClass.newBuilder().build() method.

Phase 0

1. Identify all classes/constructors/methods that need updating.
2. If there's a code style guide, we should add what we're doing to it. If there isn't, we should create one.
3. (Optional) Look into tools available for Kotlin/Java that document API changes between versions.

Phase 1

1. Add a @Deprecated annotation to all constructors/methods that are currently public, but shouldn't be.
2. Add KDoc to each affected class indicating the recommended way of constructing that class.

Phase 2 (next major version?)

1. Remove @Deprecated annotations added in Phase 1.
2. Make constructors/methods internal that should be.

[adam-arold]

This is a great idea. I'm guilty of not making it official (that you can only build these things in the RightWay™). I'd vote for

It has an internal constructor, a corresponding builder class (which itself has an internal constructor), and the way you construct the instance is by calling a public TheClass.newBuilder().build() method.

This leads to very clean and readable code: TheClass.newBuilder().build()

[adam-arold]

A note on internal: it won't hide the methods for Java consumers, for that you need to use @JvmSynthetic, but it is not recommended by the Kotlin guidelines so we might want to leave them internal.

[nanodeath]

I thought `internal` methods were obfuscated in Java?

…

On Thu, Apr 29, 2021 at 11:59 AM Adam Arold ***@***.***> wrote: A note on internal: it won't hide the methods for Java consumers, for that you need to use @JvmSynthetic, but it is not recommended by the Kotlin guidelines so we might want to leave them internal. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#382 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACTAJNNXXKZA67SJDTOGDTLGUANANCNFSM43XN3UHQ> .

[adam-arold]

Unfortunately not :( They will be visible as public in the bytecode. We can keep using internal though, it is your call.

[nanodeath]

Well, we'll do the best we can, plus it'll be documented. Kinda ugly but we could even mark the constructors as @Deprecated if we wanted to. I'm not a big fan of that though; even though we try to make things nice for Java users, there's only so far I'm willing to go on that.

[adam-arold]

Let's keep the internal then and leave the rest as-is.