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.

Sign up for GitHub

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

Fixes LinkedHashMap::values() #1656 #1657

Merged
merged 1 commit into from
Nov 2, 2016
Merged

Fixes LinkedHashMap::values() #1656 #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion javaslang/src/main/java/javaslang/collection/LinkedHashMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ public java.util.LinkedHashMap<K, V> toJavaMap() {

@Override
public Seq<V> values() {
return map.values();
return map(t -> t._2);
}

@Override
Expand Down
10 changes: 8 additions & 2 deletions javaslang/src/test/java/javaslang/collection/LinkedHashMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,14 @@ protected <K extends Comparable<? super K>, V> LinkedHashMap<K, V> mapFill(int n

@Test
public void shouldKeepOrder() {
CharSeq actual = LinkedHashMap.<Integer, Character> empty().put(3, 'a').put(2, 'b').put(1, 'c').foldLeft(CharSeq.empty(), (s, t) -> s.append(t._2));
assertThat(actual).isEqualTo(CharSeq.of("abc"));
List<Character> actual = LinkedHashMap.<Integer, Character> empty().put(3, 'a').put(2, 'b').put(1, 'c').foldLeft(List.empty(), (s, t) -> s.append(t._2));
Assertions.assertThat(actual).isEqualTo(List.of('a', 'b', 'c'));
Copy link
Member Author

Choose a reason for hiding this comment

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

It was a big mistake to override assers* methods. They don't check ordering of iterables. It is sufficient for Map, but very dangerous for other cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes - also IntMap and IntMultimap hide methods overridden by Map/Multimap because IntMap only extends Traversable. It is not sufficient that they are backed by Map/Multimap. This is the reason we did not recognize a bug...

With 3.0.0 we will change the collection type hierarchy 'a bit'

  • Iterator will not be Iterable/Traversable anymore, instead it will be TraversableOnce
  • Map/Multimap will extend Traversable<V> instead of Traversable<K, V>

This will make IntMap/IntMultimap superfluous. But for the overridden assert* methods I have currently no better solution...

Copy link
Contributor

@danieldietrich danieldietrich Nov 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be simpler to have String instead of List?

String actual = LinkedHashMap....mkString();
Assertions.assertThat(actual).isEqualTo("(3, a)(2, b)(1, c)");

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the reason we did not recognize a bug...

No, this bug was not recognized because of there was not test for it :) But we could not notice the bug in fold(), if it were

}

@Test
public void shouldKeepValuesOrder() {
List<Character> actual = LinkedHashMap.<Integer, Character> empty().put(3, 'a').put(2, 'b').put(1, 'c').values().foldLeft(List.empty(), List::append);
Assertions.assertThat(actual).isEqualTo(List.of('a', 'b', 'c'));
}

// -- static narrow
Expand Down