Adds the IonTextWriterBuilder_1_1 interface.

[tgregg]

Description of changes:

Adds an Ion 1.1 text writer builder, accessible via IonEncodingVersion.ION_1_1.textWriterBuilder(). The diff looks big but it's mostly just from moving stuff around.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[tgregg]

Extended by _Private_IonTextWriterBuilder_1_0 and _Private_IonTextWriterBuilder_1_1.

[tgregg]

This block of red moved to _Private_IonTextWriterBuilder_1_0 unchanged.

[tgregg]

The contents of this class were moved from _Private_IonTextWriterBuilder unchanged.

[tgregg]

The red methods in here were moved to the new IonWriterBuilder_1_1 interface unchanged.

[tgregg]

Most of the methods in here are copied from IonTextWriterBuilder. We duplicate them here because 1.1 users are expected to interact with the IonTextWriterBuilder_1_1, not the IonTextWriterBuilder abstract class.

[tgregg]

Initial IVM is not optional for Ion 1.1, so these tests are not applicable for the 1.1 writer. They are moved into IonTextWriterBuilder_1_0_Test.

[popematt]

LGTM as a preview of some Ion 1.1 functionality. None of my comments on the code are really blockers.

Do you think you could run ./gradlew ktlintFormat to fix the ktlint failures?

[popematt]

Suggestion—you could change to _Private_IonTextWriterBuilder<BuilderType> and have everything here return BuilderType rather than _Private_IonTextWriterBuilder. That might avoid some of the clashing return types.

[tgregg]

This is a good suggestion, and I've applied it in two steps, corresponding two the two commits in this PR that can be viewed separately.

1. Adding a parameter to _Private_IonTextWriterBuilder for the concrete type, which enables some minor cleanups.
2. Adding a parameter to the existing public IonTextWriterBuilder abstract class, allowing the concrete type to be plumbed all the way through. This allows us to remove all of the cast-only overrides in _Private_IonTextWriterBuilder_1_1. The downside is that it will add warnings to existing code that assigns to variables with the type IonTextWriterBuilder, because they'll be missing the type parameter. I'm not sure whether we'd like to consider this a breaking change or not. The code still runs, as confirmed by the tests. We can always drop the second of the two commits if we don't want to move forward with this.

[popematt]

I think adding a parameter to the existing public class/interface needs to be considered a breaking change. However, I think you can probably still get rid of the cast-only overrides if you make all of the methods in IonTextWriterBuilder abstract and move their implementations into _Private_TextWriterBuilder<T>.

I know that in general, that's even more breaking than the change you have made, but the javadoc for IonTextWriterBuilder does say:

This class should not be extended by code outside of this library."

[tgregg]

I think I'll just drop the second of the two commits currently proposed. I agree that we should avoid adding a parameter so that we don't break builds that fail if warnings are encountered. Unfortunately getting rid of cast-only overrides isn't possible even if we make the IonTextWriterBuilder methods abstract because of the duplication (with different declared return types) between IonTextWriterBuilder/_Private_IonTextWriterBuilder (and its ancestors) and the new IonTextWriterBuilder_1_1 interface.

[popematt]

Is IVM minimizing actually a useful option for Ion 1.1? We always have to start with the IVM.
Same thing with the LST minimizing. If we're just minimizing the symbol table, then this seems redundant with SymbolInliningStrategy. However, if we're minimizing the macro table as well... does that mean we're doing write-time evaluation of macros so that we can write the data without E-Expressions?

I don't think there's necessarily any action item from this comment, but it's something to keep in mind.

[tgregg]

I think IVM minimization is potentially applicable (though I'm not sure it's that useful, even in Ion 1.0), because it allows us to de-duplicate multiple IVMs that occur in a row.

For LST/macro table minimization, I see this option as on by default (because they're almost never needed explicitly), but could be disabled. Let's follow up on this before finalizing Ion 1.1 to make sure we don't have options included in the builder that we never intend to support, or that are made redundant by other options that we add.

[toddjonker]

I'm curious what alternative API designs were considered. Adding version-specific sub-interfaces feels bulky, on a surface read.

Following an established pattern in this library, another approach would be to add 1.1-specific features to a Facet. Or perhaps just on the same interface, with runtime constraints holding things consistent.

[toddjonker]

FWIW, this feels like its treading into IonSystem territory, and that's a facade class I deeply regret, since it couples too many things together. The replacement builder-pattern was intended to avoid coupling across different features, to ease things like tree-shaking and partitioning the library into smaller portions.

A simpler approach, IMO, would be to add things like IonBinaryWriterBuilder.standard11().

[popematt]

@toddjonker, here's the line of reasoning:

Regarding version-specific sub interfaces... we are trying to work within the constraints of what already exists in ion-java, and at the same time, we are trying to get away from all of the behavioral inheritance (extends) and try to just use API inheritance (implements).

All of the interfaces are here to support various use cases. I think we need to have a general IonWriterBuilder interface, as well as an interface for Ion 1.0 builders and Ion 1.1 builders, and interfaces for text builders and binary builders. That way, you can write code that e.g. expects an Ion 1.1 builder but is generic over whether it's text or binary, or e.g. expects a text builder but is agnostic to whether it is Ion 1.0 or 1.1. Finally, we need to have the version/encoding specific interfaces so that someone can get a builder that has exactly the options that are relevant to the Ion encoding that they desire.

It does feel like a lot of different interfaces, but I don't think it's unreasonable. What makes it seem unreasonable is the way we've had to shove it into the existing abstract-class inheritance structure of the builders.

As for the IonEncodingVersion class:

We considered having a withMinorVersion(int version) option on the builder. However, that was flawed because

• we would have to validate whether the specified version is valid when we could just use the type system to prevent someone from passing an invalid version
• withMinorVersion will become awkward if there's ever a new major version of Ion.
• it always returns the same type of (version-agnostic) builder

So, we considered having a withIonVersion(IonEncodingVersion version) where IonEncodingVersion is an enum. That solves some of the issues, but also is problematic because adding a new enum variant is a potentially breaking change for languages (looking at you Kotlin) that try to enforce exhaustive matches on enums.

So then we considered a "pseudo" enum—i.e. a class with a private constructor and some public constants that are effectively the enum variants.

Finally, we realized that we could go one step further and just use the IonEncodingVersion instances as the entry point to version-specific builder APIs.

If this pattern gets negative feedback from our customers, we can create methods such as standard11(), but we've been trying to avoid that so far because 11 looks like 11 rather than 1.1, and adding underscores, i.e. standard_1_1(), looks very not-Java.

We could also create methods such as this (but probably in Java):

fun <TextBuilder> standardText(version: IonVersion<TextBuilder, *>): TextBuilder
fun <BinaryBuilder> standardBinary(version: IonVersion<*, BinaryBuilder>): TextBuilder

TLDR; IonEncodingVersion is just a way to represent the Ion version as an object/type rather than as an integer (or pair of integers). It can be a static entry point to builders, but should never become more than that.