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

Correct, scala.collection.Set trait, make correct examples in ++ description #9519

Closed
scabug opened this issue Oct 15, 2015 · 23 comments
Closed

Comments

@scabug
Copy link

scabug commented Oct 15, 2015

http://www.scala-lang.org/api/current/index.html#scala.collection.immutable.Set

Set use is not shown in the use case section.

def
++[B](that: GenTraversableOnce[B]): Set[B]
[use case]
Returns a new immutable set containing the elements from the left hand operand followed by the elements from the right hand operand. The element type of the immutable set is the most specific superclass encompassing the element types of the two operands.

Example:

scala> val a = List(1)
a: List[Int] = List(1)

scala> val b = List(2)
b: List[Int] = List(2)

scala> val c = a ++ b
c: List[Int] = List(1, 2)

scala> val d = List('a')
d: List[Char] = List(a)

scala> val e = c ++ d
e: List[AnyVal] = List(1, 2, a)
@scabug
Copy link
Author

scabug commented Oct 15, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9519?orig=1
Reporter: @janekdb
Affected Versions: 2.11.7
See #8394

@scabug
Copy link
Author

scabug commented Nov 9, 2015

@soc said (edited on Nov 9, 2015 12:40:07 PM UTC):
I think the use-case itself doesn't make much sense
a) the use-case is more complex than the "real" signature and
b) it introduces a spurious type parameter.
Maybe we could get rid of the use-case instead?

@scabug
Copy link
Author

scabug commented Nov 10, 2015

Harit Himanshu (hhimanshu) said (edited on Nov 10, 2015 2:46:11 AM UTC):
It is coming from GenTraversableLike.

*  @usecase def ++[B](that: GenTraversableOnce[B]): $Coll[B]
   *    @inheritdoc
   *
   *    Example:
   *    {{{
   *      scala> val a = List(1)
   *      a: List[Int] = List(1)
   *
   *      scala> val b = List(2)
   *      b: List[Int] = List(2)
   *
   *      scala> val c = a ++ b
   *      c: List[Int] = List(1, 2)
   *
   *      scala> val d = List('a')
   *      d: List[Char] = List(a)
   *
   *      scala> val e = c ++ d
   *      e: List[AnyVal] = List(1, 2, a)
   *    }}}
   *
   *    @return       a new $coll which contains all elements of this $coll
   *                  followed by all elements of `that`.
   */
  def ++[B >: A, That](that: GenTraversableOnce[B])(implicit bf: CanBuildFrom[Repr, B, That]): That

and there a a lot more subclasses for this class

Known Subclasses
::, AbstractBuffer, AbstractIterable, AbstractIterable, AbstractMap, AbstractMap, AbstractMap, AbstractSeq, AbstractSeq, AbstractSet, AbstractSet, AbstractTraversable, AnyRefMap, Appended, Appended, Appended, Appended, ArrayBuffer, ArrayLike, ArrayOps, ArraySeq, ArrayStack, BitSet, BitSet, BitSet, BitSet1, BitSet2, BitSetLike, BitSetN, Buffer, BufferLike, BufferProxy, Cons, DefaultKeySet, DefaultKeySet, DefaultKeySet, DefaultKeySortedSet, DefaultKeySortedSet, DefaultMap, DefaultMap, DefaultMapModel, DefaultValuesIterable, DefaultValuesIterable, DoubleLinkedList, DoubleLinkedListLike, DroppedWhile, DroppedWhile, DroppedWhile, DroppedWhile, DroppedWhile, Empty, EmptyView, EmptyView, EmptyView, EmptyView, Exclusive, Filtered, Filtered, Filtered, Filtered, Filtered, FilteredKeys, FilteredKeys, FlatMapped, FlatMapped, FlatMapped, FlatMapped, Forced, Forced, Forced, Forced, GenIterable, GenIterableLike, GenMap, GenMapLike, GenSeq, GenSeqLike, GenSet, GenSetLike, GenTraversable, HashMap, HashMap, HashMap1, HashSet, HashSet, HashTrieMap, HashTrieSet, History, ImmutableDefaultKeySet, ImmutableMapAdaptor, ImmutableSetAdaptor, Impl, Inclusive, Inclusive, IndexedSeq, IndexedSeq, IndexedSeq, IndexedSeqLike, IndexedSeqLike, IndexedSeqOptimized, IndexedSeqOptimized, IndexedSeqView, IntMap, Iterable, Iterable, Iterable, IterableForwarder, IterableLike, IterableProxy, IterableProxyLike, IterableView, IterableViewLike, LinearSeq, LinearSeq, LinearSeq, LinearSeqLike, LinearSeqOptimized, LinkedHashMap, LinkedHashSet, LinkedList, LinkedListLike, List, ListBuffer, ListMap, ListMap, ListSet, LongMap, LongMap, Map, Map, Map, Map, Map1, Map2, Map3, Map4, MapLike, MapLike, MapLike, MapProxy, MapProxy, MapProxy, MapProxyLike, Mapped, Mapped, Mapped, Mapped, MappedValues, MappedValues, MultiMap, MutableList, Nil, Node, Node, NumericRange, ObservableBuffer, ObservableMap, ObservableSet, OpenHashMap, PagedSeq, ParArray, ParHashMap, ParHashMap, ParHashSet, ParHashSet, ParIterable, ParIterable, ParIterable, ParIterableLike, ParMap, ParMap, ParMap, ParMapLike, ParMapLike, ParRange, ParSeq, ParSeq, ParSeq, ParSeqLike, ParSet, ParSet, ParSet, ParSetLike, ParSetLike, ParTrieMap, ParVector, Patched, Patched, Prepended, Prepended, PriorityQueue, PriorityQueueProxy, Queue, Queue, QueueProxy, Range, ResizableArray, Reversed, Reversed, Reversed, RevertibleHistory, Script, Seq, Seq, Seq, SeqForwarder, SeqLike, SeqLike, SeqProxy, SeqProxyLike, SeqView, SeqViewLike, Set, Set, Set, Set1, Set2, Set3, Set4, SetLike, SetLike, SetProxy, SetProxy, SetProxy, SetProxyLike, Sliced, Sliced, Sliced, Sliced, Sliced, SortedMap, SortedMap, SortedMapLike, SortedSet, SortedSet, SortedSet, SortedSetLike, Stack, Stack, StackProxy, Stream, StreamView, StreamViewLike, StringBuilder, StringLike, StringOps, SynchronizedBuffer, SynchronizedMap, SynchronizedPriorityQueue, SynchronizedQueue, SynchronizedSet, SynchronizedStack, SystemProperties, TakenWhile, TakenWhile, TakenWhile, TakenWhile, TakenWhile, Transformed, Transformed, Transformed, Transformed, Transformed, Traversable, Traversable, Traversable, TraversableForwarder, TraversableLike, TraversableProxy, TraversableProxyLike, TraversableView, TraversableViewLike, TreeMap, TreeSet, TreeSet, TrieMap, UnrolledBuffer, ValueSet, Vector, WithDefault, WithDefault, WithDefault, WithDefault, WithDefault, WithDefault, WrappedArray, WrappedString, Zipped, Zipped, Zipped, ZippedAll, ZippedAll, ZippedAll, ofBoolean, ofBoolean, ofByte, ofByte, ofChar, ofChar, ofDouble, ofDouble, ofFloat, ofFloat, ofInt, ofInt, ofLong, ofLong, ofRef, ofRef, ofShort, ofShort, ofUnit, ofUnit

Would we have to fix all of them? or do we need to stop @inheritdoc the use-case (which may not be very useful)

Ideas?

@scabug
Copy link
Author

scabug commented Nov 10, 2015

@soc said:
I think it would make sense to look at some of the other use-cases, and figure out if they have the same issues.

I think it could be limited to Set though, because they are invariant ...

@scabug
Copy link
Author

scabug commented Nov 12, 2015

Harit Himanshu (hhimanshu) said (edited on Nov 12, 2015 3:03:05 PM UTC):
@simon, what is the best way to find out other use-case s?

@scabug
Copy link
Author

scabug commented Nov 12, 2015

@soc said:
You are already on the right track with the "known subclasses" thing. Just have a look at them, and figure out whether the use-case makes sense for the rest or if it is just as confusing as for Set. :-)

@scabug
Copy link
Author

scabug commented Nov 22, 2015

Harit Himanshu (hhimanshu) said:
All the known subclasses have it. I don't think they are confusing, but if we remove them, there are chances that beginners(like me) may not know on how to use them.

@scabug
Copy link
Author

scabug commented Nov 23, 2015

@SethTisue said:
would substituting $Coll for "List" in the example would make it correct for all the subclasses?

@scabug
Copy link
Author

scabug commented Nov 26, 2015

Harit Himanshu (hhimanshu) said (edited on Nov 26, 2015 5:08:50 AM UTC):
I guess yes, since as per Collection api

It is convenient to treat all collections as either a scala.collection.Traversable or scala.collection.Iterable, as these traits define the vast majority of operations on a collection.

Where Traversable is extends GenTraversable.

Would $Coll would update as per the current API someone is looking. For example, in case of List, List will be shown but in case of Set, Set will be displayed?

Please let me know if this understanding is correct, I would try to make the change and send out the pull request?

Also, is there a way to validate by generating documentation locally? Your help is very much appreciated.

@scabug
Copy link
Author

scabug commented Nov 26, 2015

@SethTisue said:

Would `$Coll` would update as per the current API someone is looking. For example, in case of List, List will be shown but in case of Set, Set will be displayed?

It certainly looks that way to me — seems worth a try.

is there a way to validate by generating documentation locally?

yes, ant docs.lib will generate the doc for the standard library into build/scaladoc/library

@scabug
Copy link
Author

scabug commented Nov 26, 2015

Harit Himanshu (hhimanshu) said:
Hey @seth, I tried doing the following

 *  @usecase def ++[B](that: GenTraversableOnce[B]): $Coll[B]
   *    @inheritdoc
   *
   *    Example:
   *    {{{
   *      scala> val a = $Coll(1)
   *      a: $Coll[Int] = $Coll(1)
   *
   *      scala> val b = $Coll(2)
   *      b: $Coll[Int] = $Coll(2)
   *
   *      scala> val c = a ++ b
   *      c: $Coll[Int] = $Coll(1, 2)
   *
   *      scala> val d = $Coll('a')
   *      d: $Coll[Char] = $Coll(a)
   *
   *      scala> val e = c ++ d
   *      e: $Coll[AnyVal] = $Coll(1, 2, a)
   *    }}}

when I build scaladoc ant docs.lib, and locate the documentation locally, I see the following

GenTraversableLike

scala> val a = `GenTraversable`(1)
a: `GenTraversable`[Int] = `GenTraversable`(1)

scala> val b = `GenTraversable`(2)
b: `GenTraversable`[Int] = `GenTraversable`(2)

scala> val c = a ++ b
c: `GenTraversable`[Int] = `GenTraversable`(1, 2)

scala> val d = `GenTraversable`('a')
d: `GenTraversable`[Char] = `GenTraversable`(a)

scala> val e = c ++ d
e: `GenTraversable`[AnyVal] = `GenTraversable`(1, 2, a)

List

scala> val a = `List`(1)
a: `List`[Int] = `List`(1)

scala> val b = `List`(2)
b: `List`[Int] = `List`(2)

scala> val c = a ++ b
c: `List`[Int] = `List`(1, 2)

scala> val d = `List`('a')
d: `List`[Char] = `List`(a)

scala> val e = c ++ d
e: `List`[AnyVal] = `List`(1, 2, a)

With my limited understanding, I started looking at other classes which provide examples and realized that every example that has been shown is between " { { { " and " } } } " and the code between them is not using $Coll to de-reference any Collection, they rather use actual Collection class.

Am I missing anything?
What is your advice?

@scabug
Copy link
Author

scabug commented Nov 30, 2015

Harit Himanshu (hhimanshu) said:
Any further advices? what shall I do now?

@scabug
Copy link
Author

scabug commented Nov 30, 2015

@SethTisue said:
sorry for my slow response; there was a U.S. holiday, and now I may not have time to look at this until later this week

@scabug
Copy link
Author

scabug commented Dec 1, 2015

Harit Himanshu (hhimanshu) said:
No worries @seth, I will wait for your next response.

@scabug
Copy link
Author

scabug commented Dec 18, 2015

@SethTisue said:
I don't think there is any wholly satisfactory solution here. It looks like the $Coll idea won't work.

I think I would suggest leaving the use case in, but simply removing the example from it. Incorrect examples are worse than no examples.

(I'm tempted to suggest removing the use case from GenTraversableLike and trying putting separate versions on SeqLike, SetLike, and MapLike that use Seq, Set, and Map in place of $Coll, but the result wouldn't be completely correct, just not quite as egregiously wrong.)

@VladUreche what do you think? should we just remove the example?

@scabug
Copy link
Author

scabug commented May 20, 2016

Harit Himanshu (hhimanshu) said:
Did we decide on anything @seth?

@scabug
Copy link
Author

scabug commented Aug 4, 2016

@SethTisue said:
sorry for the long delay — I have been on leave for the last few months.

Since Vlad hasn't chosen to weigh in, my last recommendation still stands: “I would suggest leaving the use case in, but simply removing the example from it. Incorrect examples are worse than no examples."

@scabug
Copy link
Author

scabug commented Oct 13, 2016

Jim Van Horn (jwvh) said (edited on Oct 13, 2016 5:00:35 AM UTC):
Same code/document, related issue. Line 253:

@usecase def ++[B](that: GenTraversableOnce[B]): $Coll[B]

"$Coll" is not correct for Option. Some(a) ++ Some(b) does not return an Option.

This is assuming the Std Lib code is correct and the documentation is wrong, not vice versa.

@scabug
Copy link
Author

scabug commented Oct 13, 2016

@SethTisue said:
Jim: that's #8394

@scabug
Copy link
Author

scabug commented Oct 13, 2016

@SethTisue said:
someone want to submit a PR removing the example? (to the 2.12.x branch, for inclusion in 2.12.1?)

@scabug
Copy link
Author

scabug commented Oct 16, 2016

Aravindh S (aravindhs) said:
@seth Tisue I would like to do the change. Just to confirm the change, I should remove the complete usecase section of the method in the GenTraversableLike.scala file right?

@scabug
Copy link
Author

scabug commented Oct 17, 2016

@SethTisue said:
That's what I'm suggesting, yeah.

@scabug
Copy link
Author

scabug commented Feb 4, 2017

@Philippus said:
scala/scala#5674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants