Skip to content
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 MapStream#concat() methods and factory method taking two maps #1808

Merged
merged 7 commits into from
Oct 4, 2018

Conversation

headphonejack
Copy link
Contributor

No description provided.

- Add examples to `toMap(mergeFn)` for how it can be used to merge immutable maps with preference
@headphonejack headphonejack changed the title Add MapStream#concat() methods Add MapStream#concat() methods and factory method taking two maps Oct 3, 2018
# Conflicts:
#	modules/collect/src/test/java/com/opengamma/strata/collect/MapStreamTest.java
Copy link
Member

@jodastephen jodastephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the generics, you will have to write some test cases to show passing Map<Integer, Double> to Map<Number, Object> for example

* @param other the other mapstream from which to get the entries to append
* @return the stream of entries including those newly concatenated
*/
public MapStream<K, V> concat(MapStream<K, V> other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the generics could be better. Shouldn't it be a MapStream<? extends K, ? extends V>

* @param otherMap the other map from which to get the entries to append
* @return the stream of entries including those newly concatenated
*/
public MapStream<K, V> concat(Map<K, V> otherMap) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there are many ways to create a MapStream, I wasn't thinking of having this method. Callers can just wrap the map themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not have a static method similar to Stream.concat that takes Map<? extends K, ? extends V> ...maps and an overload that takes streams i.e Stream<Map.Entry<? extends K, ? extends V>> ...mapStreams?

I can't imagine a situation where one would call the instance method over the static method, especially since streams are single use so you wouldn't want to be passing a MapStream around.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instance method is slightly more flexible (can create a MapStream from any of the factory methods then concat) and less verbose to use, but a static method is more consistent with Streams/Stream. I'd be okay with making it static if we think it's neater

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here for why Stream has a static concat method. Reluctantly, I agree with that conclusion, so I think we should follow the JDK exactly here. A concat(MapStream, MapStream) static method with generics similar to the JDK method.

Lets avoid other variants, such as ones with Map or varargs at the moment - they can be added later if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm made some initial these changes in this vein, but still looking at passing Map<Integer, Double> to Map<Number, Object> for example - the type signature of Stream.concat seems to be restrictive in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added that test. With the signature as it stands, I couldn't get round callers having to choose between:
ImmutableMap<Object, ? extends Number> result = MapStream.concat(MapStream.of(map1), MapStream.of(map2)).toMap((a,b) -> a);
or
ImmutableMap<Object, Number> result = MapStream.<Object, Number>concat(MapStream.of(map1), MapStream.of(map2)).toMap((a,b) -> a);

If you have any ideas for avoiding either one of these I'm all ears

/**
* Concatenates the other mapstream's entries to the stream.
*
* @param other the other mapstream from which to get the entries to append
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two spaces between "other" and "the" in Strata, and on all other @param tags

* @param secondMap the other map
* @return a stream of map entries derived from the values in the maps
*/
public static <K, V> MapStream<K, V> of(Map<K, V> firstMap, Map<K, V> secondMap) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the fence about this one, given the addition of concat. Again, the generics would need tweaking if keeping it.

@@ -185,6 +171,27 @@
return new MapStream<>(Stream.empty());
}

/**
* Creates a stream of map entries whose elements are those of the first map stream followed by those of the second.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the underlying Stream.concat, the order is only maintained if both inputs are ordered, so you can't promise "followed by" unless both inputs are ordered.

There isn't an obvious way to force ordering - I guess it would require buffering the input streams - so I guess we just don't worry about the problem.

MapStream<? extends K, ? extends V> a,
MapStream<? extends K, ? extends V> b) {

@SuppressWarnings("unchecked")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this annotation on to the method (seems most annotations in Strata are on the method)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general approach is to put it on the variable if there is only one usage in the method. Doesn't always happen that way, but that should be the goal. So no need to change here.

@jodastephen jodastephen merged commit 8f2ef57 into master Oct 4, 2018
@jodastephen jodastephen deleted the topic/mapstream-concat branch October 4, 2018 12:59
@jodastephen jodastephen added this to the v2.1 milestone Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants