Allow non-type converter instances in ArrayConverter and HashValueConverter

[HertzDevil]

Resolves part of #10637. Also adds specs for these converters since for some reason there weren't any.

The old class methods simply delegate to a new converter instance (e.g. ArrayConverter(T).from_json -> ArrayConverter::WithInstance(T.class).new(T).from_json); this has negligible impact on execution time, and LLVM can probably optimize out the temporary struct. We could deprecate the class methods later if that makes the interface cleaner.

[straight-shoota]

Couldn't we just keep the class methods as is?

[asterite]

Couldn't we just keep the class methods as is?

Aren't they the same? 🤔

Though technically this is a breaking change because of going from module to struct, but I doubt someone is reopening those types.

[HertzDevil]

There is at least one piece of code on GitHub that repeats the standard library's JSON::ArrayConverter verbatim. I can't find any other examples of reopening that converter. In fact I can't find any uses of JSON::HashValueConverter on GitHub outside Crystal itself and the json_mapping shard.

Would absolutely no breaking changes like this PR and the Nil return type ones be allowed for the rest of 1.x? What should be the alternative solution if we want this PR not to be one of them?

[asterite]

Would absolutely no breaking changes like this PR and the Nil return type ones be allowed for the rest of 1.x?

I think so

What should be the alternative solution if we want this PR not to be one of them?

Introduce another type that provides the new features.

I personally don't like it, but I think that's the way forward if we want to avoid breaking changes.

[HertzDevil]

Any suggestions for the new type's name?

[asterite]

ArrayConverterWithInstance or something like that.

[straight-shoota]

I would see a possibility to merge such a change. If it changes someones code, that's because they're using the API not in the way it is intended. We probably won't be able to stay 100% clear of breaking changes anyways, because fixing bugs can always mean breaking things.

[asterite]

I think in this case it's probably fine to break things. I don't think these types are used a lot, as shown by that GitHub search. And we can always provide an upgrade path if needed, or actually help those shards in need. I have no idea why another shard would define those same types, maybe that's where the idea came from, or they couldn't wait for a release to have those types.

[HertzDevil]

What do you think about having a new class method ArrayConverter.with_instance that returns a converter of a private type, similar to what we do with most methods in Iterator?

fixing bugs can always mean breaking things.

This is what I fear about; any bug fix can be considered a breaking change if any old code relies on the buggy behaviour prior to the fix.

[straight-shoota]

ArrayConverter.with_instance could just be ArrayConverter.new and still return an instance of a private type.

But, I'd prefer merging the module to struct conversion.

[asterite]

Oh, adding new to the module is a great idea! I think it has the same effect, and it's not a breaking change.

We could change it to a struct, if really needed, for 2.0