Ordering.explicit(List<T> valuesInOrder) should use Collection

[GoogleCodeExporter]

Consider to change List interface to Collection in Ordering.explicit(List<T> 
valuesInOrder) method. Because i could have a LinkedHashSet as ordered 
collection, for example. 

Original issue reported on code.google.com by abracham...@gmail.com on 25 Sep 2013 at 11:54

[GoogleCodeExporter]

I lost this argument on February 6, 2009. I still think I'm right :) Kevin, 
have your views softened at all? At least 19 of the 131 internal users of 
Ordering.explicit(List) have to convert their input to a List. And that's just 
the ones who do it on the same line. Some naive searching revealed at least a 
few others, so I'd estimate that we're talking at least 20% of all callers.

Original comment by cpov...@google.com on 25 Sep 2013 at 4:05

• Added labels: Package-Collect, Type-Enhancement

[GoogleCodeExporter]

Oh. I think it's better to use Iterable interface instead of Collection in this 
method.

Original comment by abracham...@gmail.com on 26 Sep 2013 at 7:41

[GoogleCodeExporter]

I think if you change / add overload for Ordering.explicit(Iterable<T>), you 
should add caveat for cases like Ordering.explicit(Sets.newHashSet(1,2,3)) 
where result will could be unexpected (and that's probably why List won in 2009 
;)).

Original comment by xaerx...@gmail.com on 3 Oct 2013 at 7:07

[GoogleCodeExporter]

Yep, that's exactly why it won :) Maybe nowadays with 
<https://code.google.com/p/error-prone/>, we could set up a check at least for 
internal Google users and other error-prone users, that no one is passing a 
HashSet (unless it's a LinkedHashSet). But I guess that wouldn't catch most 
users, who probably store the HashSet in a variable of type Set. Nor would it 
catch hashMap.keySet() or other non-ordered sets.

Original comment by cpov...@google.com on 3 Oct 2013 at 1:40

[GoogleCodeExporter]

Out of curiosity, is there a reason not to use ImmutableSet instead of 
LinkedHashSet?  Then you could just pass its .asList() view.

Original comment by kevinb@google.com on 11 Oct 2013 at 4:31

[GoogleCodeExporter]

Kevin looked through the callers that I dug up. Half were false positives -- 
callers who already has a list but copied it anyway, callers whose list came 
from newArrayList(E...) (who could call the existing varargs Ordering.explicit 
overload), etc. Of the remainder, a chunk were callers of 
immutableCollection.asList(), which isn't too burdensome. The rest were a 
smattering of different things, but the summary is that I feel like only two of 
them get shortchanged by the List requirement. That's a small enough niche that 
it's hard to justify adding the new overload, especially since we'd ideally 
want to remove the old overload (but probably wouldn't bother).

Original comment by cpov...@google.com on 14 Oct 2013 at 8:36

• Changed state: WorkingAsIntended

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<issue id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:12

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:08

• Added labels: Restrict-AddIssueComment-Commit