SequenceWriter.writeAll() could accept Iterable

[jkremser]

Currently the method from ${subject} looks like this:

public <C extends Collection<?>> SequenceWriter writeAll(C container) throws IOException {
        for (Object value : container) {
            write(value);
        }
        return this;
    }

The only assumption on the container is therefore it can be iterated through it using the for each loop. That being said, the signature can be weaken to accept also the ? extends Iterable. Could you please change it to

public <I extends Iterable<?>> SequenceWriter writeAll(I iterable) throws IOException {
        for (Object value : iterable) {
            write(value);
        }
        return this;
    }

Collection extends Iterable, hence no API-breakage.

[cowtowncoder]

Yes, this would probably have been better signature.

Unfortunately, such change would break binary compatibility, I think, despite not breaking source compatibility. This is actually a bigger problem since projects often (try to) upgrade dependencies, and this can break transitive dependencies (or direct ones if not recompiling).

I wonder if it would be possible to add both methods, as their type erasures are not identical.
Possible downside would be if casts were required in source to indicate which method was to be called.

[Shredder121]

There are no casts required, overload resolution process takes the most specific type possible.
Since the two methods would then do the same thing (the Collection variant can delegate to the Iterable variant) there is no problem in having two.
I'd say that at least adding a comment why there are two is recommended, until the next bigger release.

[cowtowncoder]

@Shredder121 I said binary compatibility may be a problem, not source compatibility. This because whereas Java compiler does consider overloaded variants, class loader simply matches exact type signatures. If existing method is changed, this will effectively remove existing method signature and lead to linkage error on class loading, if code was compiled against older version, but loaded with newer Jackson version. This is problematic for transitive dependencies where recompilation does not happen.

This means that existing method needs to remain in place even if new method is added.
If that works from source perspective it just means that old method can be kept, but deprecated, and new one added.

[Shredder121]

I also meant binary compatibility, the old one would still be there (my second sentence).

However I don't think @Deprecated is a good idea, since then you would need to cast to circumvent depreciation warnings.

Anyway, it's of course your call, I just wanted to mention that it is posible, no problem.

[cowtowncoder]

@Shredder121 Ok. Yes, as long as old method is kept.

Good point on needing casts, actually, had the relationship wrong way around (that is, in which compiler would have chosen new method). You are right, since the old version is more specific and would match most calls, it would still be used.

Would be nice to have a way to incrementally remove the old version, but that is probably not worth bothering about until 3.x.

[cowtowncoder]

One possibility would be to use different name, like writeEach(); this would allow deprecation.
But not sure that's worth the hassle.

[Shredder121]

I agree, I think it's fine like this now.
Keep up the good work! 👍