Add missing methods to ImmutableMapFactory

[kedar-joshi]

Additional changes include -

1. Replace usage of newly deprecated methods.
2. Update interface Javadoc to align with deprecated
   implementation in (ImmutableMapFactoryImpl#ofMap).

Closes gh-738

Signed-off-by: Kedar Joshi kdar_joshi@yahoo.com

[kedar-joshi]

I think I should also add @Since on new methods. Right ?

[nikhilnanivadekar]

@kedar-joshi thanks for your contribution! Before I review the PR, a little bit of admin stuff so that it is easier for me when I write the release notes. Can you break the additional changes which you have done in separate commits, please? It is fine to have it in this PR, just separate commits, so that I can look at the history easily and compile the release notes. Also, the history remains clean.
Yes, please add since tag as well.
Please ping me when it is done, I will review it. Cheers!
Thanks for helping us out!

[kedar-joshi]

Can you break the additional changes which you have ..

Sure.

[nikhilnanivadekar]

There are build failures. Also the implementation is far easier than what you have right now. Please take a look in the review. Thanks for your contribution!

[motlin]

I'm marking this as "request changes" so we don't merge it yet, but let's chat about it. I'm not sure we should go this route.

[motlin]

ImmutableMapFactoryImpl has a single deprecated method.

    /**
     * @deprecated use {@link #ofAll(Map)} instead (inlineable)
     */
    @Override
    @Deprecated
    public <K, V> ImmutableMap<K, V> ofMap(Map<K, V> map)
    {
        return this.ofAll(map);
    }

This does not bring the interface in line with the implementation. The one method that ought to be deprecated doesn't match the correct suggestion.

[motlin]

I feel like the change you're making is different than what the title says. You're really adding a new method called withMap and deprecating ofAll and withAll at the same time in favor of it.

[motlin]

In addition, this only changes ImmutableMapFactory. It misses other object maps (BiMap, hashing strategy) and primitive maps.

[kedar-joshi]

I feel like the change you're making is different than what the title says. You're really adding a new method called withMap and deprecating ofAll and withAll at the same time in favor of it.

@motlin I kept phrasing of the title similar to the original issue gh-738.

[nikhilnanivadekar]

Looping in @donraab as he initially opened the issue.

[motlin]

I see the context of gh-738 now, but the deprecation still doesn't make sense as written. The methods with bad generics are deprecated, but the new methods also have bad generics. There's no version of ofMap that's not deprecated.

I think we should experiment with pulling methods up to a super-interface to drive consistency (in a separate commit) to make this sort of thing enforced by the compiler.

[motlin]

Here's an example, to start the conversation: motlin@f9397b2

[motlin]

And here's a similar example for Maps. This one doesn't compile because the interfaces are truly not compatible.
motlin@07c81aa

[anishlukk123]

is this pull request abadoned?