deephaven / deephaven-core

Deephaven Community Core
Other
247 stars 79 forks source link

Documentation for Immutable Value / Builder classes #5931

Open devinrsmith opened 1 month ago

devinrsmith commented 1 month ago

Currently, the majority of our https://immutables.github.io/ builders don't have any javadoc; we (mostly) document the getter properties of the objects to be constructed.

As a simplified / abbreviated example:

/**
 * The server configuration.
 */
public interface ServerConfig {

    /**
     * The network interface this server binds to as an IP address or a hostname. If not set, then bind to all
     * interfaces.
     */
    Optional<String> host();

    /**
     * The port.
     */
    int port();

    ...

    /**
     * How many do we wait to shut down the server. Defaults to 10 seconds.
     */
    @Default
    default Duration shutdownTimeout() {
        return Duration.ofMillis(DEFAULT_SHUTDOWN_TIMEOUT_MILLIS);
    }

    ...

    interface Builder {
        Builder host(String host);

        Builder port(int port);

        ...

        Builder shutdownTimeout(Duration shutdownTimeout);

        ServerConfig build();
    }

From the javadoc on the object's class, users might be able to infer the meaning and optionality of Builder#host, Builder#port, and Builder#shutdownTimeout.

Some options:

  1. Have equivalent (duplicated) javadoc on on the the objects and builder methods
  2. Document object methods, add {@link ...} from builder methods to object methods
  3. Document builder methods, add {@link ...} from object methods to builder methods
  4. Explore exposing code-generated builders as directly public (right now, the generated code is package private); this would be similar to option 2, but we would automatically inherit {@link ...} from the generated code. As an example, here is what the auto-generated javadoc from immutables looks like:
    /**
     * Initializes the value for the {@link ServerConfig#shutdownTimeout() shutdownTimeout} attribute.
     * <p><em>If not set, this attribute will have a default value as returned by the initializer of {@link ServerConfig#shutdownTimeout() shutdownTimeout}.</em>
     * @param shutdownTimeout The value for shutdownTimeout 
     * @return {@code this} builder for use in a chained invocation
     */
    @CanIgnoreReturnValue 
    public final Builder shutdownTimeout(Duration shutdownTimeout) {
devinrsmith commented 1 month ago

I'll note: consumers of objects may be best served by documentation on the object methods, and producers of objects may be best served by documentation on the builder methods.

My general preference is to reduce duplication of documentation by providing {@link ...} where appropriate, in the spirit of reducing the chance of drift (somebody updates documentation on an object method but forgets to update the documentation on the builder method, or vice versa).

devinrsmith commented 1 month ago

Also, see original comments from https://github.com/deephaven/deephaven-core/pull/5752/files#r1706006738