search

mkodekar / guava-libraries

Automatically exported from code.google.com/p/guava-libraries
Apache License 2.0
0 stars 0 forks source link

Lists.indexMap actually doesn't reference List #1857

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Firstly, I have no idea if you want this type of feedback or not so sorry if 
not, and I hope this is the correct place to put this.

I was reading through the recent changes to Guava and came across 
https://code.google.com/p/guava-libraries/source/detail?r=8061999c2ae5a914ae765c
1bdc373b9cc4ee70d0

This pulls out a Lists.indexMap from a number of places in the code to reduce 
duplicates. I noticed, however, that the method in question isn't actually 
related to Lists; it takes Collection instead. As I've done this type of change 
a number of times I expect it used to accept List then was loosened to accept 
Collection.

There are a number of callers of this function that convert their arguments to 
Lists without need and I'd suggest moving the method to Collections2, though 
that change may be a little more controversial.

Anyway, I just thought I'd let you know of what I saw in the off chance it had 
slipped through the normal review process.

Original issue reported on code.google.com by matt.nat...@gmail.com on 26 Sep 2014 at 8:47

GoogleCodeExporter commented 9 years ago 
You have correctly deduced the process that happened as this method was being 
written, though as an internal utility I'm not terribly concerned about exactly 
where this ends up.

I'll move it to Maps, I think.

Original comment by lowas...@google.com on 26 Sep 2014 at 5:42

GoogleCodeExporter commented 9 years ago 
For future reference, is raising a ticket for this type of feedback preferred 
over commenting on the commit?

Original comment by matt.nat...@gmail.com on 29 Sep 2014 at 12:26

GoogleCodeExporter commented 9 years ago 
Yes, it's probably preferable since commits are currently just attributed to 
whoever runs the MOE sync.

Original comment by cgdec...@gmail.com on 29 Sep 2014 at 1:38

GoogleCodeExporter commented 9 years ago

Original comment by lowas...@google.com on 2 Oct 2014 at 3:11

GoogleCodeExporter commented 9 years ago 
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:08

GoogleCodeExporter commented 9 years ago

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