-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing methods to ImmutableMapFactory #819
base: master
Are you sure you want to change the base?
Add missing methods to ImmutableMapFactory #819
Conversation
I think I should also add |
@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. |
Sure. |
...llections-api/src/main/java/org/eclipse/collections/api/factory/map/ImmutableMapFactory.java
Show resolved
Hide resolved
unit-tests/src/test/java/org/eclipse/collections/impl/factory/MapsTest.java
Outdated
Show resolved
Hide resolved
...ctions/src/main/java/org/eclipse/collections/impl/map/immutable/ImmutableMapFactoryImpl.java
Outdated
Show resolved
Hide resolved
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/** | ||
* @since 10.3.0 | ||
*/ | ||
<K, V> ImmutableMap<K, V> withMapIterable(MapIterable<K, V> mapIterable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @since 10.3.0 | ||
*/ | ||
@Override | ||
public <K, V> ImmutableMap<K, V> withMap(Map<K, V> map) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, this only changes ImmutableMapFactory. It misses other object maps (BiMap, hashing strategy) and primitive maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looping in @donraab as he initially opened the issue. |
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. |
Here's an example, to start the conversation: motlin@f9397b2 |
And here's a similar example for Maps. This one doesn't compile because the interfaces are truly not compatible. |
Closes gh-738 Signed-off-by: Kedar Joshi <kdar_joshi@yahoo.com>
Signed-off-by: Kedar Joshi <kdar_joshi@yahoo.com>
is this pull request abadoned? |
Additional changes include -
implementation in (ImmutableMapFactoryImpl#ofMap).
Closes gh-738
Signed-off-by: Kedar Joshi kdar_joshi@yahoo.com